diff --git a/Documentation/access-control.txt b/Documentation/access-control.txt index ca55173b31..bb92222f83 100644 --- a/Documentation/access-control.txt +++ b/Documentation/access-control.txt @@ -483,6 +483,19 @@ to create new branches. This is done by using the special you grant the users the push force permission to be able to clean up stale branches. +[[category_delete]] +=== Delete Reference + +The delete reference category controls whether it is possible to delete +references, branches or tags. It doesn't allow any other update of +references. + +Deletion of references is also possible if `Push` with the force option +is granted, however that includes the permission to fast-forward and +force-update references to exiting and new commits. Being able to push +references for new commits is bad if bypassing of code review must be +prevented. + [[category_forge_author]] === Forge Author diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/DeleteBranchIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/DeleteBranchIT.java index adc9cabf29..1c9711f2be 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/DeleteBranchIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/DeleteBranchIT.java @@ -81,6 +81,13 @@ public class DeleteBranchIT extends AbstractDaemonTest { assertDeleteSucceeds(); } + @Test + public void deleteBranchByUserWithDeletePermission() throws Exception { + grantDelete(); + setApiUser(user); + assertDeleteSucceeds(); + } + private void blockForcePush() throws Exception { block(Permission.PUSH, ANONYMOUS_USERS, "refs/heads/*").setForce(true); } @@ -89,6 +96,10 @@ public class DeleteBranchIT extends AbstractDaemonTest { grant(Permission.PUSH, project, "refs/heads/*", true, ANONYMOUS_USERS); } + private void grantDelete() throws Exception { + allow(Permission.DELETE, ANONYMOUS_USERS, "refs/*"); + } + private void grantOwner() throws Exception { allow(Permission.OWNER, REGISTERED_USERS, "refs/*"); } diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/PushTagIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/PushTagIT.java index 7681436893..01957df5e6 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/PushTagIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/PushTagIT.java @@ -101,6 +101,11 @@ public class PushTagIT extends AbstractDaemonTest { Status.REJECTED_OTHER_REASON); fastForwardTagToNewCommit(tagType, tagName, Status.REJECTED_OTHER_REASON); + allowTagDeletion(); + fastForwardTagToExistingCommit(tagType, tagName, + Status.REJECTED_OTHER_REASON); + fastForwardTagToNewCommit(tagType, tagName, Status.REJECTED_OTHER_REASON); + allowPushOnRefsTags(); Status expectedStatus = tagType == ANNOTATED ? Status.REJECTED_OTHER_REASON : Status.OK; @@ -130,6 +135,11 @@ public class PushTagIT extends AbstractDaemonTest { Status.REJECTED_OTHER_REASON); forceUpdateTagToNewCommit(tagType, tagName, Status.REJECTED_OTHER_REASON); + allowTagDeletion(); + forceUpdateTagToExistingCommit(tagType, tagName, + Status.REJECTED_OTHER_REASON); + forceUpdateTagToNewCommit(tagType, tagName, Status.REJECTED_OTHER_REASON); + allowForcePushOnRefsTags(); forceUpdateTagToExistingCommit(tagType, tagName, Status.OK); forceUpdateTagToNewCommit(tagType, tagName, Status.OK); @@ -155,6 +165,13 @@ public class PushTagIT extends AbstractDaemonTest { String tagName = pushTagForExistingCommit(tagType, Status.OK); pushTagDeletion(tagType, tagName, Status.OK); } + + removePushFromRefsTags(); + allowTagDeletion(); + for (TagType tagType : TagType.values()) { + String tagName = pushTagForExistingCommit(tagType, Status.OK); + pushTagDeletion(tagType, tagName, Status.OK); + } } private String pushTagForExistingCommit(TagType tagType, @@ -251,6 +268,11 @@ public class PushTagIT extends AbstractDaemonTest { grant(Permission.PUSH, project, "refs/tags/*", true, REGISTERED_USERS); } + private void allowTagDeletion() throws Exception { + removePushFromRefsTags(); + grant(Permission.DELETE, project, "refs/tags/*", true, REGISTERED_USERS); + } + private void removePushFromRefsTags() throws Exception { removePermission(Permission.PUSH, project, "refs/tags/*"); } 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 97f11b44b4..9fc58b49f4 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 @@ -24,6 +24,7 @@ public class Permission implements Comparable { public static final String ABANDON = "abandon"; public static final String ADD_PATCH_SET = "addPatchSet"; public static final String CREATE = "create"; + public static final String DELETE = "delete"; public static final String DELETE_DRAFTS = "deleteDrafts"; public static final String EDIT_HASHTAGS = "editHashtags"; public static final String EDIT_TOPIC_NAME = "editTopicName"; @@ -56,6 +57,7 @@ public class Permission implements Comparable { NAMES_LC.add(ABANDON.toLowerCase()); NAMES_LC.add(ADD_PATCH_SET.toLowerCase()); NAMES_LC.add(CREATE.toLowerCase()); + NAMES_LC.add(DELETE.toLowerCase()); NAMES_LC.add(FORGE_AUTHOR.toLowerCase()); NAMES_LC.add(FORGE_COMMITTER.toLowerCase()); NAMES_LC.add(FORGE_SERVER.toLowerCase()); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AdminConstants.properties b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AdminConstants.properties index 2fe5978531..d75a68dde7 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AdminConstants.properties +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AdminConstants.properties @@ -123,6 +123,7 @@ permissionNames = \ abandon, \ addPatchSet, \ create, \ + delete, \ deleteDrafts, \ editHashtags, \ editTopicName, \ @@ -145,6 +146,7 @@ permissionNames = \ abandon = Abandon addPatchSet = Add Patch Set create = Create Reference +delete = Delete Reference deleteDrafts = Delete Drafts editHashtags = Edit Hashtags editTopicName = Edit Topic Name 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 bd98a29886..8fbf06f543 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 @@ -213,7 +213,27 @@ public class RefControl { /** @return true if the user can rewind (force push) the reference. */ public boolean canForceUpdate() { - return (canPushWithForce() || canDelete()) && canWrite(); + if (!canWrite()) { + return false; + } + + if (canPushWithForce()) { + return true; + } + + switch (getUser().getAccessPath()) { + case GIT: + return false; + + case JSON_RPC: + case REST_API: + case SSH_COMMAND: + case UNKNOWN: + case WEB_BROWSER: + default: + return getUser().getCapabilities().canAdministrateServer() + || (isOwner() && !isForceBlocked(Permission.PUSH)); + } } public boolean canWrite() { @@ -356,7 +376,7 @@ public class RefControl { switch (getUser().getAccessPath()) { case GIT: - return canPushWithForce(); + return canPushWithForce() || canPerform(Permission.DELETE); case JSON_RPC: case REST_API: @@ -366,7 +386,8 @@ public class RefControl { default: return getUser().getCapabilities().canAdministrateServer() || (isOwner() && !isForceBlocked(Permission.PUSH)) - || canPushWithForce(); + || canPushWithForce() + || canPerform(Permission.DELETE); } }