opendev/gerrit
Code Issues Proposed changes

Revert "Create Review Permission"

Browse Source 
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 97afc05387.

Change-Id: I450b50b57f06ddabefc3c08cd2680ff7023ca0fd
This commit is contained in:
Dave Borowitz
2018-04-24 12:48:09 -04:00
parent 942a8fe8cf
commit aa91227531
14 changed files with 52 additions and 144 deletions
Download Patch File Download Diff File Expand all files Collapse all files

46
Documentation/access-control.txt
View File

@@ -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

2
gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AdminConstants.properties
View File

@@ -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

7
java/com/google/gerrit/common/data/Permission.java
View File

@@ -24,7 +24,6 @@ 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";
@@ -56,7 +55,6 @@ 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());
@@ -171,11 +169,6 @@ 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());

2
java/com/google/gerrit/server/git/receive/ReceiveCommits.java
View File

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

12
java/com/google/gerrit/server/permissions/ProjectControl.java
View File

@@ -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() {

2
java/com/google/gerrit/server/permissions/RefControl.java
View File

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

37
java/com/google/gerrit/server/project/ProjectConfig.java
View File

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

2
java/com/google/gerrit/server/schema/AllProjectsCreator.java
View File

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

8
javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
View File

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

3
javatests/com/google/gerrit/acceptance/git/PushPermissionsIT.java
View File

@@ -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",

2
javatests/com/google/gerrit/acceptance/rest/change/MoveChangeIT.java
View File

@@ -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 =

12
javatests/com/google/gerrit/server/permissions/RefControlTest.java
View File

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

57
javatests/com/google/gerrit/server/project/ProjectConfigTest.java
View File

@@ -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 =

4
polygerrit-ui/app/behaviors/gr-access-behavior/gr-access-behavior.html
View File

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