From 4666bd958dc898d277be50e368e1b17c251c47f0 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Wed, 7 Sep 2016 11:43:59 +0200 Subject: [PATCH] Add separate 'Delete Reference' permission This new permission allows users to delete branches and tags, without allowing them to push branches and tags for new commits and thus bypass code review, as it would be possible when force push would be granted. Bug: Issue 2179 Change-Id: I678650570ed2ce14fc1784fc7766b8ddbe7f9729 Signed-off-by: Edwin Kempin --- Documentation/access-control.txt | 13 +++++++++ .../rest/project/DeleteBranchIT.java | 11 ++++++++ .../acceptance/rest/project/PushTagIT.java | 22 +++++++++++++++ .../google/gerrit/common/data/Permission.java | 2 ++ .../client/admin/AdminConstants.properties | 2 ++ .../gerrit/server/project/RefControl.java | 27 ++++++++++++++++--- 6 files changed, 74 insertions(+), 3 deletions(-) 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); } }