diff --git a/Documentation/access-control.txt b/Documentation/access-control.txt index 397b99a742..55049de50c 100644 --- a/Documentation/access-control.txt +++ b/Documentation/access-control.txt @@ -555,6 +555,22 @@ 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 @@ -589,29 +605,6 @@ 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 @@ -895,7 +888,7 @@ any changes. Suggested access rights to grant: * xref:category_read[`Read`] on 'refs/heads/\*' and 'refs/tags/*' -* xref:category_push[`Push`] to 'refs/for/refs/heads/*' +* xref:category_create_review[`Create Review`] to '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 @@ -923,7 +916,7 @@ exclusively. Suggested access rights to grant: * xref:category_read[`Read`] on 'refs/heads/\*' and 'refs/tags/*' -* xref:category_push[`Push`] to 'refs/for/refs/heads/*' +* xref:category_push[`Create Review`] to '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/*' @@ -984,8 +977,7 @@ 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[`Push`] to 'refs/for/refs/heads/*' - +* xref:category_push[`Create Review`] to '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 8d6878fecb..b29d5cf261 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,6 +135,7 @@ permissionNames = \ abandon, \ addPatchSet, \ create, \ + createReview, \ createTag, \ createSignedTag, \ delete, \ @@ -158,6 +159,7 @@ 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 b82b9310b5..de20397a7f 100644 --- a/java/com/google/gerrit/common/data/Permission.java +++ b/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 CREATE_REVIEW = "createReview"; public static final String CREATE_SIGNED_TAG = "createSignedTag"; public static final String CREATE_TAG = "createTag"; public static final String DELETE = "delete"; @@ -55,6 +56,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(CREATE_REVIEW.toLowerCase()); NAMES_LC.add(CREATE_SIGNED_TAG.toLowerCase()); NAMES_LC.add(CREATE_TAG.toLowerCase()); NAMES_LC.add(DELETE.toLowerCase()); @@ -169,6 +171,11 @@ 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 6b541ba1d9..442f604d49 100644 --- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java +++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java @@ -231,7 +231,7 @@ class ReceiveCommits { + "'Force Push' flag set to delete references."), DELETE_CHANGES("Cannot delete from '" + REFS_CHANGES + "'"), CODE_REVIEW( - "You need 'Push' rights to upload code review requests.\n" + "You need 'Create Review' 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 dbd60ea5c9..dfc75cec27 100644 --- a/java/com/google/gerrit/server/permissions/ProjectControl.java +++ b/java/com/google/gerrit/server/permissions/ProjectControl.java @@ -162,6 +162,7 @@ class ProjectControl { boolean canPushToAtLeastOneRef() { return canPerformOnAnyRef(Permission.PUSH) || canPerformOnAnyRef(Permission.CREATE_TAG) + || canPerformOnAnyRef(Permission.CREATE_REVIEW) || isOwner(); } @@ -207,16 +208,7 @@ class ProjectControl { } private boolean canCreateChanges() { - 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; + return canPerformOnAnyRef(Permission.CREATE_REVIEW); } private boolean isDeclaredOwner() { diff --git a/java/com/google/gerrit/server/permissions/RefControl.java b/java/com/google/gerrit/server/permissions/RefControl.java index 28781ea454..6a581016e4 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 projectControl.controlForRef("refs/for/" + refName).canPerform(Permission.PUSH); + return canPerform(Permission.CREATE_REVIEW); } /** @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 15bc54c3c9..d2fd7cef7d 100644 --- a/java/com/google/gerrit/server/project/ProjectConfig.java +++ b/java/com/google/gerrit/server/project/ProjectConfig.java @@ -703,6 +703,13 @@ 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()); } @@ -722,6 +729,36 @@ 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 a91d5e641e..fe4b2758a7 100644 --- a/java/com/google/gerrit/server/schema/AllProjectsCreator.java +++ b/java/com/google/gerrit/server/schema/AllProjectsCreator.java @@ -167,6 +167,7 @@ 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) { @@ -194,7 +195,6 @@ 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 83c6768a43..24f3048a64 100644 --- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java +++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java @@ -902,7 +902,7 @@ public class ChangeIT extends AbstractDaemonTest { } @Test - public void rebaseNotAllowedWithoutPushPermission() throws Exception { + public void rebaseNotAllowedWithoutCreateReviewPermission() throws Exception { // Create two changes both with the same parent PushOneCommit.Result r = createChange(); testRepo.reset("HEAD~1"); @@ -914,7 +914,7 @@ public class ChangeIT extends AbstractDaemonTest { revision.submit(); grant(project, "refs/heads/master", Permission.REBASE, false, REGISTERED_USERS); - block("refs/for/*", Permission.PUSH, REGISTERED_USERS); + block("refs/*", Permission.CREATE_REVIEW, REGISTERED_USERS); // Rebase the second String changeId = r2.getChangeId(); @@ -925,7 +925,7 @@ public class ChangeIT extends AbstractDaemonTest { } @Test - public void rebaseNotAllowedForOwnerWithoutPushPermission() throws Exception { + public void rebaseNotAllowedForOwnerWithoutCreateReviewPermission() throws Exception { // Create two changes both with the same parent PushOneCommit.Result r = createChange(); testRepo.reset("HEAD~1"); @@ -936,7 +936,7 @@ public class ChangeIT extends AbstractDaemonTest { revision.review(ReviewInput.approve()); revision.submit(); - block("refs/for/*", Permission.PUSH, REGISTERED_USERS); + block("refs/*", Permission.CREATE_REVIEW, REGISTERED_USERS); // Rebase the second String changeId = r2.getChangeId(); diff --git a/javatests/com/google/gerrit/acceptance/rest/change/MoveChangeIT.java b/javatests/com/google/gerrit/acceptance/rest/change/MoveChangeIT.java index 93ad2fed73..7ff274dbcf 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 moveChangeToBranchWithoutUploadPerms() throws Exception { + public void moveChangeToBranchWithoutCreateReviewPerms() 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 97889fe7dc..7e671dfa13 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; @@ -390,10 +390,10 @@ public class RefControlTest { @Test public void inheritRead_SingleBranchDeniesUpload() { allow(parent, READ, REGISTERED_USERS, "refs/*"); - allow(parent, PUSH, REGISTERED_USERS, "refs/for/refs/*"); + allow(parent, CREATE_REVIEW, REGISTERED_USERS, "refs/*"); allow(local, READ, REGISTERED_USERS, "refs/heads/foobar"); doNotInherit(local, READ, "refs/heads/foobar"); - doNotInherit(local, PUSH, "refs/for/refs/heads/foobar"); + doNotInherit(local, CREATE_REVIEW, "refs/heads/foobar"); ProjectControl u = user(local); assertCanUpload(u); @@ -403,7 +403,7 @@ public class RefControlTest { @Test public void blockPushDrafts() { - allow(parent, PUSH, REGISTERED_USERS, "refs/for/refs/*"); + allow(parent, CREATE_REVIEW, REGISTERED_USERS, "refs/*"); block(parent, PUSH, ANONYMOUS_USERS, "refs/drafts/*"); allow(local, PUSH, REGISTERED_USERS, "refs/drafts/*"); @@ -432,7 +432,7 @@ public class RefControlTest { @Test public void inheritRead_SingleBranchDoesNotOverrideInherited() { allow(parent, READ, REGISTERED_USERS, "refs/*"); - allow(parent, PUSH, REGISTERED_USERS, "refs/for/refs/*"); + allow(parent, CREATE_REVIEW, REGISTERED_USERS, "refs/*"); allow(local, READ, REGISTERED_USERS, "refs/heads/foobar"); ProjectControl u = user(local); @@ -503,7 +503,7 @@ public class RefControlTest { public void cannotUploadToAnyRef() { allow(parent, READ, REGISTERED_USERS, "refs/*"); allow(local, READ, DEVS, "refs/heads/*"); - allow(local, PUSH, DEVS, "refs/for/refs/heads/*"); + allow(local, CREATE_REVIEW, DEVS, "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 0e4ba10415..63338c45a8 100644 --- a/javatests/com/google/gerrit/server/project/ProjectConfigTest.java +++ b/javatests/com/google/gerrit/server/project/ProjectConfigTest.java @@ -18,6 +18,7 @@ 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; @@ -557,6 +558,62 @@ 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 fb0c685a58..6c251a697b 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,6 +40,10 @@ limitations under the License. id: 'create', name: 'Create Reference', }, + createReview: { + id: 'createReview', + name: 'Create Review', + }, createTag: { id: 'createTag', name: 'Create Annotated Tag',