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
diff --git a/Documentation/access-control.txt b/Documentation/access-control.txt
index 397b99a..55049de 100644
--- a/Documentation/access-control.txt
+++ b/Documentation/access-control.txt
@@ -555,6 +555,22 @@
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 @@
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 @@
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 @@
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 @@
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 8d6878f..b29d5cf 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 @@
abandon, \
addPatchSet, \
create, \
+ createReview, \
createTag, \
createSignedTag, \
delete, \
@@ -158,6 +159,7 @@
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 b82b931..de20397 100644
--- a/java/com/google/gerrit/common/data/Permission.java
+++ b/java/com/google/gerrit/common/data/Permission.java
@@ -24,6 +24,7 @@
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 @@
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 @@
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());
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index 6b541ba..442f604 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 @@
+ "'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 dbd60ea..dfc75ce 100644
--- a/java/com/google/gerrit/server/permissions/ProjectControl.java
+++ b/java/com/google/gerrit/server/permissions/ProjectControl.java
@@ -162,6 +162,7 @@
boolean canPushToAtLeastOneRef() {
return canPerformOnAnyRef(Permission.PUSH)
|| canPerformOnAnyRef(Permission.CREATE_TAG)
+ || canPerformOnAnyRef(Permission.CREATE_REVIEW)
|| isOwner();
}
@@ -207,16 +208,7 @@
}
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 28781ea..6a58101 100644
--- a/java/com/google/gerrit/server/permissions/RefControl.java
+++ b/java/com/google/gerrit/server/permissions/RefControl.java
@@ -148,7 +148,7 @@
}
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 15bc54c..d2fd7ce 100644
--- a/java/com/google/gerrit/server/project/ProjectConfig.java
+++ b/java/com/google/gerrit/server/project/ProjectConfig.java
@@ -703,6 +703,13 @@
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 @@
}
}
+ 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 a91d5e6..fe4b275 100644
--- a/java/com/google/gerrit/server/schema/AllProjectsCreator.java
+++ b/java/com/google/gerrit/server/schema/AllProjectsCreator.java
@@ -167,6 +167,7 @@
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 @@
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 83c6768..24f3048 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 @@
}
@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 @@
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 @@
}
@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 @@
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 93ad2fe..7ff274d 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 @@
}
@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 97889fe..7e671df 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 @@
@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 @@
@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 @@
@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 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 0e4ba10..63338c4 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.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,6 +559,62 @@
}
@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 =
tr.commit()
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 fb0c685..6c251a6 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 @@
id: 'create',
name: 'Create Reference',
},
+ createReview: {
+ id: 'createReview',
+ name: 'Create Review',
+ },
createTag: {
id: 'createTag',
name: 'Create Annotated Tag',
