Merge branch 'stable-2.14' into stable-2.15
* stable-2.14:
Add new "Delete Changes" permission
ChangeIT: Refactor tests for deletion of new changes
Change-Id: I6eda2f9901118fe7eadab9811258fd042e2349a0
diff --git a/Documentation/access-control.txt b/Documentation/access-control.txt
index 92eebee..67a4c13 100644
--- a/Documentation/access-control.txt
+++ b/Documentation/access-control.txt
@@ -841,6 +841,17 @@
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 3804734d..fb94b67 100644
--- a/Documentation/user-review-ui.txt
+++ b/Documentation/user-review-ui.txt
@@ -258,7 +258,9 @@
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.
+permission, if they are granted the
+link:access-control.html#category_delete_changes[Delete Changes] permission,
+or if they are an administrator.
** [[plugin-actions]]Further actions may be available if plugins are installed.
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 aa05ea2..ebb66bb 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
@@ -44,6 +44,7 @@
import static com.google.gerrit.server.StarredChangesUtil.DEFAULT_LABEL;
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;
@@ -89,6 +90,8 @@
import com.google.gerrit.extensions.api.groups.GroupApi;
import com.google.gerrit.extensions.api.projects.BranchInput;
import com.google.gerrit.extensions.api.projects.ConfigInput;
+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;
@@ -927,12 +930,7 @@
@Test
public void deleteNewChangeAsAdmin() throws Exception {
- PushOneCommit.Result changeResult = createChange();
- String changeId = changeResult.getChangeId();
-
- gApi.changes().id(changeId).delete();
-
- assertThat(query(changeId)).isEmpty();
+ deleteChangeAsUser(admin, admin);
}
@Test
@@ -949,51 +947,79 @@
}
@Test
- @TestProjectInput(cloneAs = "user")
- public void deleteChangeAsUserWithDeleteOwnChangesPermissionForGroup() throws Exception {
- allow("refs/*", Permission.DELETE_OWN_CHANGES, REGISTERED_USERS);
- deleteChangeAsUser();
+ public void deleteNewChangeAsUserWithDeleteChangesPermissionForGroup() throws Exception {
+ allow("refs/*", Permission.DELETE_CHANGES, REGISTERED_USERS);
+ deleteChangeAsUser(admin, user);
}
@Test
- @TestProjectInput(cloneAs = "user")
- public void deleteChangeAsUserWithDeleteOwnChangesPermissionForOwners() throws Exception {
- allow("refs/*", Permission.DELETE_OWN_CHANGES, CHANGE_OWNER);
- deleteChangeAsUser();
+ 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);
}
- private void deleteChangeAsUser() throws Exception {
- try {
- PushOneCommit.Result changeResult =
- pushFactory.create(db, user.getIdent(), testRepo).to("refs/for/master");
- String changeId = changeResult.getChangeId();
- int id = changeResult.getChange().getId().id;
- RevCommit commit = changeResult.getCommit();
+ @Test
+ public void deleteChangeAsUserWithDeleteOwnChangesPermissionForGroup() throws Exception {
+ allow("refs/*", Permission.DELETE_OWN_CHANGES, REGISTERED_USERS);
+ deleteChangeAsUser(user, user);
+ }
- setApiUser(user);
+ @Test
+ public void deleteChangeAsUserWithDeleteOwnChangesPermissionForOwners() throws Exception {
+ allow("refs/*", Permission.DELETE_OWN_CHANGES, CHANGE_OWNER);
+ deleteChangeAsUser(user, user);
+ }
+
+ 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 = projectName.get();
+ in.branch = "refs/heads/master";
+ in.subject = "test";
+ ChangeInfo changeInfo = gApi.changes().create(in).get();
+ String changeId = changeInfo.changeId;
+ int id = changeInfo._number;
+ String commit = changeInfo.currentRevision;
+
+ assertThat(gApi.changes().id(changeId).info().owner._accountId).isEqualTo(owner.id.get());
+
+ setApiUser(deleteAs);
gApi.changes().id(changeId).delete();
assertThat(query(changeId)).isEmpty();
String ref = new Change.Id(id).toRefPrefix() + "1";
- eventRecorder.assertRefUpdatedEvents(project.get(), ref, null, commit, commit, null);
+ eventRecorder.assertRefUpdatedEvents(projectName.get(), ref, null, commit, commit, null);
} finally {
removePermission(project, "refs/*", Permission.DELETE_OWN_CHANGES);
+ removePermission(project, "refs/*", Permission.DELETE_CHANGES);
}
}
@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();
+ deleteChangeAsUser(user, admin);
}
@Test
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 7afc123..4910424 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 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_OWN_CHANGES = "deleteOwnChanges";
public static final String EDIT_HASHTAGS = "editHashtags";
public static final String EDIT_ASSIGNEE = "editAssignee";
@@ -76,6 +77,7 @@
NAMES_LC.add(EDIT_HASHTAGS.toLowerCase());
NAMES_LC.add(EDIT_ASSIGNEE.toLowerCase());
NAMES_LC.add(DELETE_OWN_CHANGES.toLowerCase());
+ NAMES_LC.add(DELETE_CHANGES.toLowerCase());
LABEL_INDEX = NAMES_LC.indexOf(Permission.LABEL);
LABEL_AS_INDEX = NAMES_LC.indexOf(Permission.LABEL_AS.toLowerCase());
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 2a2c5fc..1582d43 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
@@ -156,8 +156,7 @@
switch (status) {
case NEW:
case ABANDONED:
- return (isOwner() && getRefControl().canDeleteOwnChanges(isOwner()))
- || getProjectControl().isAdmin();
+ return (getRefControl().canDeleteChanges(isOwner()) || getProjectControl().isAdmin());
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 e024731..cde2c80 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
@@ -285,9 +285,10 @@
return canPerform(Permission.VIEW_PRIVATE_CHANGES);
}
- /** @return true if this user can delete their own changes. */
- boolean canDeleteOwnChanges(boolean isChangeOwner) {
- return canPerform(Permission.DELETE_OWN_CHANGES, isChangeOwner);
+ /** @return true if this user can delete changes. */
+ 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. */
