Add new "Delete Changes" permission

Changes can only be deleted by Administrators, or by the change owner
when they are granted the "Delete Own Changes" permission either on the
"Change Owners" virtual group or an internal group they are a member of.

This means that it is not possible to allow users to delete other users'
changes other than by granting the "Administrate Server" permission,
which also gives them all the other administrative capabilities. This is
also inconsistent with how the "Delete Drafts" permission works.

An example of where it would be better to have more flexible permissions
is allowing changes to be deleted by Project Owners on a per-project
basis, or by a group of users who are trusted (but not enough to grant
them Administrator capabilities).

Add a new permission "Delete Changes" which allows these use cases, thus
a change can now also be deleted by users who are:

- Member of a group that is explicitly granted "Delete Changes" on the
  change's destination branch.
- Member of a group that is given the "Owner" permission on the project,
  when the virtual group "Project Owners" is granted "Delete Changes" on
  the change's destination branch.

Bug: Issue 9354
Change-Id: I9d5d779eb0a6ce18faca02b9fc90904ec5da91d9

6 files changed, 58 insertions, 6 deletions

diff --git a/Documentation/access-control.txt b/Documentation/access-control.txt
index 9a849dcc8c..e55378f65f 100644
--- a/Documentation/access-control.txt
+++ b/Documentation/access-control.txt
@@ -887,6 +887,17 @@ the `Delete Drafts` access right assigned).
 This category permits users to delete their own changes if they are not merged
 yet. This means only own changes that are open or abandoned can be deleted.
 
+[[category_delete_changes]]
+=== Delete Changes
+
+This category permits users to delete other users' changes if they are not merged
+yet. This means only changes that are open or abandoned can be deleted.
+
+Having this permission implies having the link:#category_delete_own_changes[
+Delete Own Changes] permission.
+
+Administrators may always delete changes without having this permission.
+
 [[category_edit_topic_name]]
 === Edit Topic Name
 
diff --git a/Documentation/user-review-ui.txt b/Documentation/user-review-ui.txt
index 434a7a4c99..821ec4bff1 100644
--- a/Documentation/user-review-ui.txt
+++ b/Documentation/user-review-ui.txt
@@ -276,7 +276,9 @@ Deletes the change / the currently viewed draft patch set.
 For open or abandoned changes, the `Delete Change` button will be available
 and if the user is the change owner and is granted the
 link:access-control.html#category_delete_own_changes[Delete Own Changes]
-permission or if they are an administrator. For draft changes,
+permission, if they are granted the
+link:access-control.html#category_delete_changes[Delete Changes] permisson,
+or if they are an administrator. For draft changes,
 the `Delete Change` / `Delete Revision` buttons will be available if the user is
 the change owner or has the
 link:access-control.html#category_delete_drafts[Delete Drafts] access right assigned.
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 95bde7353f..1bde86833a 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
@@ -26,6 +26,7 @@ import static com.google.gerrit.extensions.client.ReviewerState.REVIEWER;
 import static com.google.gerrit.reviewdb.client.RefNames.changeMetaRef;
 import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS;
 import static com.google.gerrit.server.group.SystemGroupBackend.CHANGE_OWNER;
+import static com.google.gerrit.server.group.SystemGroupBackend.PROJECT_OWNERS;
 import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
 import static com.google.gerrit.server.project.Util.category;
 import static com.google.gerrit.server.project.Util.value;
@@ -64,6 +65,8 @@ import com.google.gerrit.extensions.api.changes.ReviewInput.DraftHandling;
 import com.google.gerrit.extensions.api.changes.RevisionApi;
 import com.google.gerrit.extensions.api.groups.GroupApi;
 import com.google.gerrit.extensions.api.projects.BranchInput;
+import com.google.gerrit.extensions.api.projects.ProjectApi;
+import com.google.gerrit.extensions.api.projects.ProjectInput;
 import com.google.gerrit.extensions.client.ChangeKind;
 import com.google.gerrit.extensions.client.ChangeStatus;
 import com.google.gerrit.extensions.client.Comment.Range;
@@ -460,6 +463,33 @@ public class ChangeIT extends AbstractDaemonTest {
   }
 
   @Test
+  public void deleteNewChangeAsUserWithDeleteChangesPermissionForGroup() throws Exception {
+    allow(Permission.DELETE_CHANGES, REGISTERED_USERS, "refs/*");
+    deleteChangeAsUser(admin, user);
+  }
+
+  @Test
+  public void deleteNewChangeAsUserWithDeleteChangesPermissionForProjectOwners() throws Exception {
+    GroupApi groupApi = gApi.groups().create(name("delete-change"));
+    groupApi.addMembers("user");
+
+    ProjectInput in = new ProjectInput();
+    in.name = name("delete-change");
+    in.owners = Lists.newArrayListWithCapacity(1);
+    in.owners.add(groupApi.name());
+    in.createEmptyCommit = true;
+    ProjectApi api = gApi.projects().create(in);
+
+    Project.NameKey nameKey = new Project.NameKey(api.get().name);
+
+    ProjectConfig cfg = projectCache.checkedGet(nameKey).getConfig();
+    Util.allow(cfg, Permission.DELETE_CHANGES, PROJECT_OWNERS, "refs/*");
+    saveProjectConfig(nameKey, cfg);
+
+    deleteChangeAsUser(nameKey, admin, user);
+  }
+
+  @Test
   public void deleteChangeAsUserWithDeleteOwnChangesPermissionForGroup() throws Exception {
     allow(Permission.DELETE_OWN_CHANGES, REGISTERED_USERS, "refs/*");
     deleteChangeAsUser(user, user);
@@ -472,10 +502,15 @@ public class ChangeIT extends AbstractDaemonTest {
   }
 
   private void deleteChangeAsUser(TestAccount owner, TestAccount deleteAs) throws Exception {
+    deleteChangeAsUser(project, owner, deleteAs);
+  }
+
+  private void deleteChangeAsUser(
+      Project.NameKey projectName, TestAccount owner, TestAccount deleteAs) throws Exception {
     try {
       setApiUser(owner);
       ChangeInput in = new ChangeInput();
-      in.project = project.get();
+      in.project = projectName.get();
       in.branch = "refs/heads/master";
       in.subject = "test";
       String changeId = gApi.changes().create(in).get().changeId;
@@ -488,6 +523,7 @@ public class ChangeIT extends AbstractDaemonTest {
       assertThat(query(changeId)).isEmpty();
     } finally {
       removePermission(Permission.DELETE_OWN_CHANGES, project, "refs/*");
+      removePermission(Permission.DELETE_CHANGES, project, "refs/*");
     }
   }
 
diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/Permission.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/Permission.java
index 47c5224b60..30bd089c39 100644
--- a/gerrit-common/src/main/java/com/google/gerrit/common/data/Permission.java
+++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/Permission.java
@@ -27,6 +27,7 @@ public class Permission implements Comparable<Permission> {
   public static final String DELETE = "delete";
   public static final String CREATE_TAG = "createTag";
   public static final String CREATE_SIGNED_TAG = "createSignedTag";
+  public static final String DELETE_CHANGES = "deleteChanges";
   public static final String DELETE_DRAFTS = "deleteDrafts";
   public static final String DELETE_OWN_CHANGES = "deleteOwnChanges";
   public static final String EDIT_HASHTAGS = "editHashtags";
@@ -79,6 +80,7 @@ public class Permission implements Comparable<Permission> {
     NAMES_LC.add(EDIT_ASSIGNEE.toLowerCase());
     NAMES_LC.add(DELETE_DRAFTS.toLowerCase());
     NAMES_LC.add(DELETE_OWN_CHANGES.toLowerCase());
+    NAMES_LC.add(DELETE_CHANGES.toLowerCase());
     NAMES_LC.add(PUBLISH_DRAFTS.toLowerCase());
 
     LABEL_INDEX = NAMES_LC.indexOf(Permission.LABEL);
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 9e244a5507..40be5f507a 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,7 +261,7 @@ public class ChangeControl {
         return (isOwner() || getRefControl().canDeleteDrafts());
       case NEW:
       case ABANDONED:
-        return (isAdmin() || (isOwner() && getRefControl().canDeleteOwnChanges(isOwner())));
+        return (isAdmin() || getRefControl().canDeleteChanges(isOwner()));
       case MERGED:
       default:
         return false;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java
index 5c50001137..ada1855959 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java
@@ -417,9 +417,10 @@ public class RefControl {
     return canPerform(Permission.DELETE_DRAFTS);
   }
 
-  /** @return true if this user can delete their own changes. */
-  public boolean canDeleteOwnChanges(boolean isChangeOwner) {
-    return canPerform(Permission.DELETE_OWN_CHANGES, isChangeOwner);
+  /** @return true if this user can delete changes. */
+  public boolean canDeleteChanges(boolean isChangeOwner) {
+    return canPerform(Permission.DELETE_CHANGES)
+        || (isChangeOwner && canPerform(Permission.DELETE_OWN_CHANGES, isChangeOwner));
   }
 
   /** @return true if this user can edit topic names. */