From aa912275314ff27e402787840e59081a8f9e6d8c Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Tue, 24 Apr 2018 12:48:09 -0400 Subject: [PATCH] Revert "Create Review Permission" The migration was broken; see 942a8fe8 for further rationale. Also fixes the tests that depended on this new feature: https://gerrit-review.googlesource.com/c/gerrit/+/171871/2..3/javatests/com/google/gerrit/acceptance/git/PushPermissionsIT.java This reverts commit 97afc05387137e73c1af3f9565985f4c584cacc3. Change-Id: I450b50b57f06ddabefc3c08cd2680ff7023ca0fd --- Documentation/access-control.txt | 46 ++++++++------- .../client/admin/AdminConstants.properties | 2 - .../google/gerrit/common/data/Permission.java | 7 --- .../server/git/receive/ReceiveCommits.java | 2 +- .../server/permissions/ProjectControl.java | 12 +++- .../gerrit/server/permissions/RefControl.java | 2 +- .../gerrit/server/project/ProjectConfig.java | 37 ------------ .../server/schema/AllProjectsCreator.java | 2 +- .../acceptance/api/change/ChangeIT.java | 8 +-- .../acceptance/git/PushPermissionsIT.java | 3 +- .../acceptance/rest/change/MoveChangeIT.java | 2 +- .../server/permissions/RefControlTest.java | 12 ++-- .../server/project/ProjectConfigTest.java | 57 ------------------- .../gr-access-behavior.html | 4 -- 14 files changed, 52 insertions(+), 144 deletions(-) diff --git a/Documentation/access-control.txt b/Documentation/access-control.txt index 55049de50c..397b99a742 100644 --- a/Documentation/access-control.txt +++ b/Documentation/access-control.txt @@ -555,22 +555,6 @@ global capabilities, which is the same as being able to administrate the Gerrit server (e.g. the user could assign the `Administrate Server` capability to the own account). -[[category_create_review]] -==== Create Review - -The `Create Review` access right granted on the namespace -`refs/heads/BRANCH` permits the user to upload a non-merge -commit to the project's `refs/{for,publish}/BRANCH` namespace, -creating a new change for code review. - -A user must be able to clone or fetch the project in order to create -a new commit on their local system, so in practice they must also -have the `Read` access granted to upload a change. - -For an open source, public Gerrit installation, it is common to grant -`Create Review` for `+refs/heads/*+` to `Registered Users` in the -`All-Projects` ACL. For more private installations, its common to -grant `Create Review` for `+refs/heads/*+` to all users of a project. [[category_push]] === Push @@ -605,6 +589,29 @@ its code review functionality. Projects that need to require code reviews should not grant this category. +[[category_push_review]] +==== Upload To Code Review + +The `Push` access right granted on the namespace +`refs/for/refs/heads/BRANCH` permits the user to upload a non-merge +commit to the project's `refs/for/BRANCH` namespace, creating a new +change for code review. + +A user must be able to clone or fetch the project in order to create +a new commit on their local system, so in practice they must also +have the `Read` access granted to upload a change. + +For an open source, public Gerrit installation, it is common to grant +`Push` for `+refs/for/refs/heads/*+` to `Registered Users` in the +`All-Projects` ACL. For more private installations, its common to +grant `Push` for `+refs/for/refs/heads/*+` to all users of a project. + +* Force option ++ +The force option has no function when granted to a branch in the +`+refs/for/refs/heads/*+` namespace. + + [[category_add_patch_set]] === Add Patch Set @@ -888,7 +895,7 @@ any changes. Suggested access rights to grant: * xref:category_read[`Read`] on 'refs/heads/\*' and 'refs/tags/*' -* xref:category_create_review[`Create Review`] to 'refs/heads/*' +* xref:category_push[`Push`] to 'refs/for/refs/heads/*' * link:config-labels.html#label_Code-Review[`Code-Review`] with range '-1' to '+1' for 'refs/heads/*' If it's desired to have the possibility to upload temporarily hidden @@ -916,7 +923,7 @@ exclusively. Suggested access rights to grant: * xref:category_read[`Read`] on 'refs/heads/\*' and 'refs/tags/*' -* xref:category_push[`Create Review`] to 'refs/heads/*' +* xref:category_push[`Push`] to 'refs/for/refs/heads/*' * xref:category_push_merge[`Push merge commit`] to 'refs/for/refs/heads/*' * xref:category_forge_author[`Forge Author Identity`] to 'refs/heads/*' * link:config-labels.html#label_Code-Review[`Label: Code-Review`] with range '-2' to '+2' for 'refs/heads/*' @@ -977,7 +984,8 @@ Suggested access rights to grant, that won't block changes: Optional access rights to grant: * link:config-labels.html#label_Code-Review[`Label: Code-Review`] with range '-1' to '+1' for 'refs/heads/*' -* xref:category_push[`Create Review`] to 'refs/heads/*' +* xref:category_push[`Push`] to 'refs/for/refs/heads/*' + [[examples_integrator]] === Integrator 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 b29d5cf261..8d6878fecb 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 @@ -135,7 +135,6 @@ permissionNames = \ abandon, \ addPatchSet, \ create, \ - createReview, \ createTag, \ createSignedTag, \ delete, \ @@ -159,7 +158,6 @@ permissionNames = \ abandon = Abandon addPatchSet = Add Patch Set create = Create Reference -createReview = Create Review createTag = Create Annotated Tag createSignedTag = Create Signed Tag delete = Delete Reference diff --git a/java/com/google/gerrit/common/data/Permission.java b/java/com/google/gerrit/common/data/Permission.java index de20397a7f..b82b9310b5 100644 --- a/java/com/google/gerrit/common/data/Permission.java +++ b/java/com/google/gerrit/common/data/Permission.java @@ -24,7 +24,6 @@ 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 CREATE_REVIEW = "createReview"; public static final String CREATE_SIGNED_TAG = "createSignedTag"; public static final String CREATE_TAG = "createTag"; public static final String DELETE = "delete"; @@ -56,7 +55,6 @@ 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(CREATE_REVIEW.toLowerCase()); NAMES_LC.add(CREATE_SIGNED_TAG.toLowerCase()); NAMES_LC.add(CREATE_TAG.toLowerCase()); NAMES_LC.add(DELETE.toLowerCase()); @@ -171,11 +169,6 @@ public class Permission implements Comparable { rules.add(rule); } - public void addAll(List rules) { - initRules(); - this.rules.addAll(rules); - } - public void remove(PermissionRule rule) { if (rule != null) { removeRule(rule.getGroup()); diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java index 6ff1ec9673..17e804fb75 100644 --- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java +++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java @@ -230,7 +230,7 @@ class ReceiveCommits { + "'Force Push' flag set to delete references."), DELETE_CHANGES("Cannot delete from '" + REFS_CHANGES + "'"), CODE_REVIEW( - "You need 'Create Review' rights to upload code review requests.\n" + "You need 'Push' rights to upload code review requests.\n" + "Verify that you are pushing to the right branch."); private final String value; diff --git a/java/com/google/gerrit/server/permissions/ProjectControl.java b/java/com/google/gerrit/server/permissions/ProjectControl.java index dfc75cec27..dbd60ea5c9 100644 --- a/java/com/google/gerrit/server/permissions/ProjectControl.java +++ b/java/com/google/gerrit/server/permissions/ProjectControl.java @@ -162,7 +162,6 @@ class ProjectControl { boolean canPushToAtLeastOneRef() { return canPerformOnAnyRef(Permission.PUSH) || canPerformOnAnyRef(Permission.CREATE_TAG) - || canPerformOnAnyRef(Permission.CREATE_REVIEW) || isOwner(); } @@ -208,7 +207,16 @@ class ProjectControl { } private boolean canCreateChanges() { - return canPerformOnAnyRef(Permission.CREATE_REVIEW); + for (SectionMatcher matcher : access()) { + AccessSection section = matcher.getSection(); + if (section.getName().startsWith("refs/for/")) { + Permission permission = section.getPermission(Permission.PUSH); + if (permission != null && controlForRef(section.getName()).canPerform(Permission.PUSH)) { + return true; + } + } + } + return false; } private boolean isDeclaredOwner() { diff --git a/java/com/google/gerrit/server/permissions/RefControl.java b/java/com/google/gerrit/server/permissions/RefControl.java index 6a581016e4..28781ea454 100644 --- a/java/com/google/gerrit/server/permissions/RefControl.java +++ b/java/com/google/gerrit/server/permissions/RefControl.java @@ -148,7 +148,7 @@ class RefControl { } private boolean canUpload() { - return canPerform(Permission.CREATE_REVIEW); + return projectControl.controlForRef("refs/for/" + refName).canPerform(Permission.PUSH); } /** @return true if this user can submit merge patch sets to this ref */ diff --git a/java/com/google/gerrit/server/project/ProjectConfig.java b/java/com/google/gerrit/server/project/ProjectConfig.java index d2fd7cef7d..15bc54c3c9 100644 --- a/java/com/google/gerrit/server/project/ProjectConfig.java +++ b/java/com/google/gerrit/server/project/ProjectConfig.java @@ -703,13 +703,6 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. groupsByName, perm, Permission.hasRange(convertedName)); - if (migrateRefsFor(as, perm)) { - if (isRefsForExclusively(refName)) { - // Since the ref only applies on refs/for/* and no other - // namespaces, we can remove the old permission. - remove(as, perm); - } - } } else { sectionsWithUnknownPermissions.add(as.getName()); } @@ -729,36 +722,6 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. } } - private boolean isRefsForExclusively(String refName) { - return refName.startsWith("refs/for/"); - } - - private boolean isRefsFor(String refName) { - return refName.startsWith("refs/for/") || refName.equals("refs/*"); - } - - private String unRefsFor(String refName) { - if (!isRefsFor(refName)) { - return refName; - } - if (refName.equals("refs/*") || refName.equals("refs/for/*")) { - return "refs/*"; - } - return refName.substring("refs/for/".length()); - } - - private boolean migrateRefsFor(AccessSection as, Permission perm) { - String refName = as.getName(); - if (isRefsFor(refName) && perm.getName().equals(Permission.PUSH)) { - AccessSection migratedAs = getAccessSection(unRefsFor(refName), true); - Permission convertedPerm = migratedAs.getPermission(Permission.CREATE_REVIEW, true); - convertedPerm.setExclusiveGroup(perm.getExclusiveGroup()); - convertedPerm.addAll(perm.getRules()); - return true; - } - return false; - } - private boolean isValidRegex(String refPattern) { try { RefPattern.validateRegExp(refPattern); diff --git a/java/com/google/gerrit/server/schema/AllProjectsCreator.java b/java/com/google/gerrit/server/schema/AllProjectsCreator.java index fe4b2758a7..a91d5e641e 100644 --- a/java/com/google/gerrit/server/schema/AllProjectsCreator.java +++ b/java/com/google/gerrit/server/schema/AllProjectsCreator.java @@ -167,7 +167,6 @@ public class AllProjectsCreator { grant(config, cap, GlobalCapability.ADMINISTRATE_SERVER, admin); grant(config, all, Permission.READ, admin, anonymous); - grant(config, all, Permission.CREATE_REVIEW, registered); grant(config, refsFor, Permission.ADD_PATCH_SET, registered); if (batch != null) { @@ -195,6 +194,7 @@ public class AllProjectsCreator { grant(config, tags, Permission.CREATE_TAG, admin, owners); grant(config, tags, Permission.CREATE_SIGNED_TAG, admin, owners); + grant(config, magic, Permission.PUSH, registered); grant(config, magic, Permission.PUSH_MERGE, registered); meta.getPermission(Permission.READ, true).setExclusiveGroup(true); diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java index 5f19d50002..40736167a5 100644 --- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java +++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java @@ -926,7 +926,7 @@ public class ChangeIT extends AbstractDaemonTest { } @Test - public void rebaseNotAllowedWithoutCreateReviewPermission() throws Exception { + public void rebaseNotAllowedWithoutPushPermission() throws Exception { // Create two changes both with the same parent PushOneCommit.Result r = createChange(); testRepo.reset("HEAD~1"); @@ -938,7 +938,7 @@ public class ChangeIT extends AbstractDaemonTest { revision.submit(); grant(project, "refs/heads/master", Permission.REBASE, false, REGISTERED_USERS); - block("refs/*", Permission.CREATE_REVIEW, REGISTERED_USERS); + block("refs/for/*", Permission.PUSH, REGISTERED_USERS); // Rebase the second String changeId = r2.getChangeId(); @@ -949,7 +949,7 @@ public class ChangeIT extends AbstractDaemonTest { } @Test - public void rebaseNotAllowedForOwnerWithoutCreateReviewPermission() throws Exception { + public void rebaseNotAllowedForOwnerWithoutPushPermission() throws Exception { // Create two changes both with the same parent PushOneCommit.Result r = createChange(); testRepo.reset("HEAD~1"); @@ -960,7 +960,7 @@ public class ChangeIT extends AbstractDaemonTest { revision.review(ReviewInput.approve()); revision.submit(); - block("refs/*", Permission.CREATE_REVIEW, REGISTERED_USERS); + block("refs/for/*", Permission.PUSH, REGISTERED_USERS); // Rebase the second String changeId = r2.getChangeId(); diff --git a/javatests/com/google/gerrit/acceptance/git/PushPermissionsIT.java b/javatests/com/google/gerrit/acceptance/git/PushPermissionsIT.java index 2811df5bba..b362a3660a 100644 --- a/javatests/com/google/gerrit/acceptance/git/PushPermissionsIT.java +++ b/javatests/com/google/gerrit/acceptance/git/PushPermissionsIT.java @@ -62,7 +62,6 @@ public class PushPermissionsIT extends AbstractDaemonTest { removeAllBranchPermissions( cfg, Permission.ADD_PATCH_SET, - Permission.CREATE_REVIEW, Permission.CREATE, Permission.DELETE, Permission.PUSH, @@ -221,7 +220,7 @@ public class PushPermissionsIT extends AbstractDaemonTest { assertThat(r) .hasMessages( "Branch refs/heads/master:", - "You need 'Create Review' rights to upload code review requests.", + "You need 'Push' rights to upload code review requests.", "Verify that you are pushing to the right branch.", "User: admin", "Please read the documentation and contact an administrator", diff --git a/javatests/com/google/gerrit/acceptance/rest/change/MoveChangeIT.java b/javatests/com/google/gerrit/acceptance/rest/change/MoveChangeIT.java index 7ff274dbcf..93ad2fed73 100644 --- a/javatests/com/google/gerrit/acceptance/rest/change/MoveChangeIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/change/MoveChangeIT.java @@ -153,7 +153,7 @@ public class MoveChangeIT extends AbstractDaemonTest { } @Test - public void moveChangeToBranchWithoutCreateReviewPerms() throws Exception { + public void moveChangeToBranchWithoutUploadPerms() throws Exception { // Move change to a destination where user doesn't have upload permissions PushOneCommit.Result r = createChange(); Branch.NameKey newBranch = diff --git a/javatests/com/google/gerrit/server/permissions/RefControlTest.java b/javatests/com/google/gerrit/server/permissions/RefControlTest.java index c54deded90..a14895bbea 100644 --- a/javatests/com/google/gerrit/server/permissions/RefControlTest.java +++ b/javatests/com/google/gerrit/server/permissions/RefControlTest.java @@ -11,10 +11,10 @@ // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. // See the License for the specific language governing permissions and // limitations under the License. + package com.google.gerrit.server.permissions; import static com.google.common.truth.Truth.assertThat; -import static com.google.gerrit.common.data.Permission.CREATE_REVIEW; import static com.google.gerrit.common.data.Permission.EDIT_TOPIC_NAME; import static com.google.gerrit.common.data.Permission.LABEL; import static com.google.gerrit.common.data.Permission.OWNER; @@ -396,10 +396,10 @@ public class RefControlTest { @Test public void inheritRead_SingleBranchDeniesUpload() { allow(parent, READ, REGISTERED_USERS, "refs/*"); - allow(parent, CREATE_REVIEW, REGISTERED_USERS, "refs/*"); + allow(parent, PUSH, REGISTERED_USERS, "refs/for/refs/*"); allow(local, READ, REGISTERED_USERS, "refs/heads/foobar"); doNotInherit(local, READ, "refs/heads/foobar"); - doNotInherit(local, CREATE_REVIEW, "refs/heads/foobar"); + doNotInherit(local, PUSH, "refs/for/refs/heads/foobar"); ProjectControl u = user(local); assertCanUpload(u); @@ -409,7 +409,7 @@ public class RefControlTest { @Test public void blockPushDrafts() { - allow(parent, CREATE_REVIEW, REGISTERED_USERS, "refs/*"); + allow(parent, PUSH, REGISTERED_USERS, "refs/for/refs/*"); block(parent, PUSH, ANONYMOUS_USERS, "refs/drafts/*"); allow(local, PUSH, REGISTERED_USERS, "refs/drafts/*"); @@ -438,7 +438,7 @@ public class RefControlTest { @Test public void inheritRead_SingleBranchDoesNotOverrideInherited() { allow(parent, READ, REGISTERED_USERS, "refs/*"); - allow(parent, CREATE_REVIEW, REGISTERED_USERS, "refs/*"); + allow(parent, PUSH, REGISTERED_USERS, "refs/for/refs/*"); allow(local, READ, REGISTERED_USERS, "refs/heads/foobar"); ProjectControl u = user(local); @@ -509,7 +509,7 @@ public class RefControlTest { public void cannotUploadToAnyRef() { allow(parent, READ, REGISTERED_USERS, "refs/*"); allow(local, READ, DEVS, "refs/heads/*"); - allow(local, CREATE_REVIEW, DEVS, "refs/heads/*"); + allow(local, PUSH, DEVS, "refs/for/refs/heads/*"); ProjectControl u = user(local); assertCannotUpload(u); diff --git a/javatests/com/google/gerrit/server/project/ProjectConfigTest.java b/javatests/com/google/gerrit/server/project/ProjectConfigTest.java index 63338c45a8..0e4ba10415 100644 --- a/javatests/com/google/gerrit/server/project/ProjectConfigTest.java +++ b/javatests/com/google/gerrit/server/project/ProjectConfigTest.java @@ -18,7 +18,6 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertWithMessage; import com.google.common.collect.Iterables; -import com.google.common.collect.Lists; import com.google.gerrit.common.data.AccessSection; import com.google.gerrit.common.data.ContributorAgreement; import com.google.gerrit.common.data.GroupReference; @@ -558,62 +557,6 @@ public class ProjectConfigTest extends GerritBaseTests { + "Raw html replacement not allowed")); } - @Test - public void readConfigPushRefsForStarIsMigrated() throws Exception { - RevCommit rev = - tr.commit() - .add("groups", group(developers)) - .add("project.config", "[access \"refs/for/*\"]\n" + " push = group Developers\n") - .create(); - - ProjectConfig cfg = read(rev); - AccessSection as = cfg.getAccessSection("refs/for/*"); - assertThat(as).isNull(); - - as = cfg.getAccessSection("refs/*"); - assertThat(as.getPermission(Permission.CREATE_REVIEW, false)).isNotNull(); - assertThat(as.getPermission(Permission.CREATE_REVIEW, false).getRules()) - .isEqualTo(Lists.newArrayList(new PermissionRule(developers))); - } - - @Test - public void readConfigPushRefsStarIsMigrated() throws Exception { - RevCommit rev = - tr.commit() - .add("groups", group(developers)) - .add("project.config", "[access \"refs/*\"]\n" + " push = group Developers\n") - .create(); - - ProjectConfig cfg = read(rev); - AccessSection as = cfg.getAccessSection("refs/*"); - assertThat(as).isNotNull(); - - as = cfg.getAccessSection("refs/*"); - assertThat(as.getPermission(Permission.CREATE_REVIEW, false)).isNotNull(); - assertThat(as.getPermission(Permission.CREATE_REVIEW, false).getRules()) - .isEqualTo(Lists.newArrayList(new PermissionRule(developers))); - } - - @Test - public void readConfigPushRefsForRefsHeadsMasterIsMigrated() throws Exception { - RevCommit rev = - tr.commit() - .add("groups", group(developers)) - .add( - "project.config", - "[access \"refs/for/refs/heads/master\"]\n" + " push = group Developers\n") - .create(); - - ProjectConfig cfg = read(rev); - AccessSection as = cfg.getAccessSection("refs/for/refs/heads/master"); - assertThat(as).isNull(); - - as = cfg.getAccessSection("refs/heads/master"); - assertThat(as.getPermission(Permission.CREATE_REVIEW, false)).isNotNull(); - assertThat(as.getPermission(Permission.CREATE_REVIEW, false).getRules()) - .isEqualTo(Lists.newArrayList(new PermissionRule(developers))); - } - @Test public void readCommentLinkMatchButNoHtmlOrLink() throws Exception { RevCommit rev = diff --git a/polygerrit-ui/app/behaviors/gr-access-behavior/gr-access-behavior.html b/polygerrit-ui/app/behaviors/gr-access-behavior/gr-access-behavior.html index 6c251a697b..fb0c685a58 100644 --- a/polygerrit-ui/app/behaviors/gr-access-behavior/gr-access-behavior.html +++ b/polygerrit-ui/app/behaviors/gr-access-behavior/gr-access-behavior.html @@ -40,10 +40,6 @@ limitations under the License. id: 'create', name: 'Create Reference', }, - createReview: { - id: 'createReview', - name: 'Create Review', - }, createTag: { id: 'createTag', name: 'Create Annotated Tag',