Create Review Permission

This is the first change in a series that intends to getting rid of
refs/for/ from ACLs.

The migration from "push refs/for/*" to "create-review refs/heads/*"
is handled on-the-fly when the configuration is loaded.

Any "push refs/for/*" permission will be  automatially migrated to the
new "Create Review" permission and the old permission will be removed
if possible (e.g. push refs/* will create a new Create Review permission,
but keep the old permission since it will affect refs/{heads,meta}/*).

When the rest of the refs/for/ dependent permissions are migrated, an
offline schema migration should be available to help correct all configs,
though the online auto migration should be available for a few major
versions to help fix ACL-mistakes by users.

Change-Id: Ia112adeb7593a7b4a7a8b5d8b96f72f73c9b70b8

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

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

java/com/google/gerrit/common/data/Permission.java

@@ -24,6 +24,7 @@ public class Permission implements Comparable<Permission> {
   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<Permission> {
     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<Permission> {
     rules.add(rule);
   }
 
+  public void addAll(List<PermissionRule> rules) {
+    initRules();
+    this.rules.addAll(rules);
+  }
+
   public void remove(PermissionRule rule) {
     if (rule != null) {
       removeRule(rule.getGroup());

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;

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()) {
+    return canPerformOnAnyRef(Permission.CREATE_REVIEW);
-      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() {

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 */

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);

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);

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();

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 =

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);

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 =

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',