opendev/gerrit
Code Issues Proposed changes

Revert migration from refs/for/* permissions to refs/heads/*

Browse Source 
The migration contains a bug which causes project.config to become
unreadable, breaking a site until it is fixed. A fix is in progress[1],
but there are still some open questions that we don't have answers to.
Revert the functionality for now, to give us time to fully bake a fix.

This reverts the following commits:
  9ff07177ea
  8392e982c0
  66c52dae95
  642f3e07e7

[1] https://gerrit-review.googlesource.com/c/gerrit/+/173513

Change-Id: I139ba7aa97a946f2c31258e7dfaa3a94ee72ff6e
This commit is contained in:
Dave Borowitz
2018-04-24 11:56:17 -04:00
parent c1e0a94b92
commit 942a8fe8cf
15 changed files with 54 additions and 234 deletions
Download Patch File Download Diff File Expand all files Collapse all files

17
Documentation/access-control.txt
View File

@@ -610,9 +610,10 @@ reviews should not grant this category.
This category controls which users are allowed to upload new patch sets to
existing changes. Irrespective of this permission, change owners are always
allowed to upload new patch sets for their changes.
allowed to upload new patch sets for their changes. This permission needs to be
set on `refs/for/*`.
By default, this permission is granted to `Registered Users` on `refs/*`,
By default, this permission is granted to `Registered Users` on `refs/for/*`,
allowing all registered users to upload a new patch set to any change. Revoking
this permission (by granting it to no groups and setting the "Exclusive" flag)
will prevent users from uploading a patch set to a change they do not own.
@@ -628,6 +629,13 @@ push to happen. Some projects wish to restrict merges to being created
by Gerrit. By granting `Push` without `Push Merge Commit`, the only
merges that enter the system will be those created by Gerrit.
The reference name connected to a `Push Merge Commit` entry must always
be prefixed with `refs/for/`, for example `refs/for/refs/heads/BRANCH`.
This applies even for an entry that complements a `Push` entry for
`refs/heads/BRANCH` that allows direct pushes of non-merge commits, and
the intention of the `Push Merge Commit` entry is to allow direct pushes
of merge commits.
[[category_push_annotated]]
[[category_create_annotated]]
@@ -787,7 +795,8 @@ above) must enable submit, and also must not block it. See above for
details on each label.
To link:user-upload.html#auto_merge[immediately submit a change on push]
the caller needs to enable the "Submit on Push" option.
the caller needs to have the Submit permission on `refs/for/<ref>`
(e.g. on `refs/for/refs/heads/master`).
Submitting to the `refs/meta/config` branch is only allowed to project
owners. Any explicit submit permissions for non-project-owners on this
@@ -908,7 +917,7 @@ 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_merge[`Push merge commit`] 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/*'
* link:config-labels.html#label_Verified[`Label: Verified`] with range '-1' to '+1' for 'refs/heads/*'

2
gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/PermissionEditor.ui.xml
View File

@@ -58,7 +58,7 @@ limitations under the License.
position: absolute;
top: 0;
right: 36px;
width: 9em;
width: 7em;
font-size: 80%;
}

25
gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/PermissionRuleEditor.java
View File

@@ -16,7 +16,6 @@ package com.google.gerrit.client.admin;
import static com.google.gerrit.common.data.Permission.EDIT_TOPIC_NAME;
import static com.google.gerrit.common.data.Permission.PUSH;
import static com.google.gerrit.common.data.Permission.SUBMIT;
import com.google.gerrit.client.Dispatcher;
import com.google.gerrit.client.Gerrit;
@@ -140,23 +139,15 @@ public class PermissionRuleEditor extends Composite
initWidget(uiBinder.createAndBindUi(this));
String name = permission.getName();
boolean canForce = true;
switch (name) {
case SUBMIT:
force.setText(PermissionRule.FORCE_SUBMIT);
break;
case EDIT_TOPIC_NAME:
force.setText(PermissionRule.FORCE_EDIT);
break;
case PUSH:
force.setText(PermissionRule.FORCE_PUSH);
String ref = section.getName();
canForce = !ref.startsWith("refs/for/") && !ref.startsWith("^refs/for/");
break;
default:
canForce = false;
boolean canForce = PUSH.equals(name);
if (canForce) {
String ref = section.getName();
canForce = !ref.startsWith("refs/for/") && !ref.startsWith("^refs/for/");
force.setText(PermissionRule.FORCE_PUSH);
} else {
canForce = EDIT_TOPIC_NAME.equals(name);
force.setText(PermissionRule.FORCE_EDIT);
}
force.setVisible(canForce);
force.setEnabled(!readOnly);
action.getElement().setPropertyBoolean("disabled", readOnly);

2
gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/PermissionRuleEditor.ui.xml
View File

@@ -55,7 +55,7 @@ limitations under the License.
position: absolute;
top: 0;
right: 36px;
width: 9em;
width: 7em;
font-size: 80%;
}

1
java/com/google/gerrit/common/data/PermissionRule.java
View File

@@ -15,7 +15,6 @@
package com.google.gerrit.common.data;
public class PermissionRule implements Comparable<PermissionRule> {
public static final String FORCE_SUBMIT = "Submit on Push";
public static final String FORCE_PUSH = "Force Push";
public static final String FORCE_EDIT = "Force Edit";

4
java/com/google/gerrit/server/permissions/ChangeControl.java
View File

@@ -141,7 +141,7 @@ class ChangeControl {
/** Can this user rebase this change? */
private boolean canRebase(ReviewDb db) throws OrmException {
return (isOwner() || refControl.canSubmit(isOwner(), false) || refControl.canRebase())
return (isOwner() || refControl.canSubmit(isOwner()) || refControl.canRebase())
&& refControl.asForRef().testOrFalse(RefPermission.CREATE_CHANGE)
&& !isPatchSetLocked(db);
}
@@ -374,7 +374,7 @@ class ChangeControl {
case RESTORE:
return canRestore(db());
case SUBMIT:
return refControl.canSubmit(isOwner(), false);
return refControl.canSubmit(isOwner());
case REMOVE_REVIEWER:
case SUBMIT_AS:

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

@@ -29,6 +29,7 @@ import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.PermissionBackend.ForChange;
import com.google.gerrit.server.permissions.PermissionBackend.ForRef;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.util.MagicBranch;
import com.google.gwtorm.server.OrmException;
import com.google.inject.util.Providers;
import java.util.Collection;
@@ -96,7 +97,9 @@ class RefControl {
/** @return true if this user can add a new patch set to this ref */
boolean canAddPatchSet() {
return canPerform(Permission.ADD_PATCH_SET);
return projectControl
.controlForRef(MagicBranch.NEW_CHANGE + refName)
.canPerform(Permission.ADD_PATCH_SET);
}
/** @return true if this user can rebase changes on this ref */
@@ -105,7 +108,7 @@ class RefControl {
}
/** @return true if this user can submit patch sets to this ref */
boolean canSubmit(boolean isChangeOwner, boolean force) {
boolean canSubmit(boolean isChangeOwner) {
if (RefNames.REFS_CONFIG.equals(refName)) {
// Always allow project owners to submit configuration changes.
// Submitting configuration changes modifies the access control
@@ -114,7 +117,7 @@ class RefControl {
// granting of powers beyond submitting to the configuration.
return projectControl.isOwner();
}
return canPerform(Permission.SUBMIT, isChangeOwner, force);
return canPerform(Permission.SUBMIT, isChangeOwner, false);
}
/** @return true if this user can force edit topic names. */
@@ -150,7 +153,7 @@ class RefControl {
/** @return true if this user can submit merge patch sets to this ref */
private boolean canUploadMerges() {
return canPerform(Permission.PUSH_MERGE);
return projectControl.controlForRef("refs/for/" + refName).canPerform(Permission.PUSH_MERGE);
}
/** @return true if the user can update the reference as a fast-forward. */
@@ -491,7 +494,7 @@ class RefControl {
return canPerform(refPermissionName(perm));
case UPDATE_BY_SUBMIT:
return canSubmit(true, true);
return projectControl.controlForRef(MagicBranch.NEW_CHANGE + refName).canSubmit(true);
case READ_PRIVATE_CHANGES:
return canPerform(Permission.VIEW_PRIVATE_CHANGES);

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

@@ -703,13 +703,19 @@ 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());
}
}
}
}
migrateRefsFor();
AccessSection capability = null;
for (String varName : rc.getNames(CAPABILITY)) {
@@ -723,20 +729,6 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
}
}
private void migrateRefsFor() {
for (AccessSection as : ImmutableList.copyOf(accessSections.values())) {
for (Permission p : ImmutableList.copyOf(as.getPermissions())) {
if (migrateRefsFor(as, p)) {
if (isRefsForExclusively(as.getName())) {
// Since the ref only applies on refs/for/* and no other
// namespaces, we can remove the old permission.
remove(as, p);
}
}
}
}
}
private boolean isRefsForExclusively(String refName) {
return refName.startsWith("refs/for/");
}
@@ -764,35 +756,6 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
convertedPerm.addAll(perm.getRules());
return true;
}
if (isRefsForExclusively(refName) && perm.getName().equals(Permission.SUBMIT)) {
// We only migrate "Submit" if is exclusively added to refs/for/.
// We do this because when the new "Submit on Push" force
// option is added, it is somewhat likely that people will start specify
// Submit on refs/* (something which previously had to be avoided if the
// user did not want to enable "Submit on push").
// Also, before this change, the documentation mentioned
// explicitly that "refs/for/*" will enable "Submit on push".
AccessSection migratedAs = getAccessSection(unRefsFor(refName), true);
Permission migratedPerm = migratedAs.getPermission(Permission.SUBMIT, true);
migratedPerm.setExclusiveGroup(perm.getExclusiveGroup());
for (PermissionRule rule : perm.getRules()) {
PermissionRule migratedRule = migratedPerm.getRule(rule.getGroup(), true);
migratedRule.setForce(true);
}
return true;
}
if (isRefsForExclusively(refName) && perm.getName().equals(Permission.ADD_PATCH_SET)) {
// No need to migrate ADD_PATCH_SET on refs/*
AccessSection migratedAs = getAccessSection(unRefsFor(refName), true);
migratedAs.addPermission(perm);
return true;
}
if (isRefsForExclusively(refName) && perm.getName().equals(Permission.PUSH_MERGE)) {
// No need to migrate PUSH_MERGE on refs/*
AccessSection migratedAs = getAccessSection(unRefsFor(refName), true);
migratedAs.addPermission(perm);
return true;
}
return false;
}

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

@@ -162,11 +162,13 @@ public class AllProjectsCreator {
AccessSection heads = config.getAccessSection(AccessSection.HEADS, true);
AccessSection tags = config.getAccessSection("refs/tags/*", true);
AccessSection meta = config.getAccessSection(RefNames.REFS_CONFIG, true);
AccessSection refsFor = config.getAccessSection("refs/for/*", true);
AccessSection magic = config.getAccessSection("refs/for/" + AccessSection.ALL, true);
grant(config, cap, GlobalCapability.ADMINISTRATE_SERVER, admin);
grant(config, all, Permission.READ, admin, anonymous);
grant(config, all, Permission.CREATE_REVIEW, registered);
grant(config, all, Permission.ADD_PATCH_SET, registered);
grant(config, refsFor, Permission.ADD_PATCH_SET, registered);
if (batch != null) {
Permission priority = cap.getPermission(GlobalCapability.PRIORITY, true);
@@ -184,7 +186,6 @@ public class AllProjectsCreator {
grant(config, heads, cr, -2, 2, admin, owners);
grant(config, heads, Permission.CREATE, admin, owners);
grant(config, heads, Permission.PUSH, admin, owners);
grant(config, all, Permission.PUSH_MERGE, registered);
grant(config, heads, Permission.SUBMIT, admin, owners);
grant(config, heads, Permission.FORGE_AUTHOR, registered);
grant(config, heads, Permission.FORGE_COMMITTER, admin, owners);
@@ -194,6 +195,8 @@ 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_MERGE, registered);
meta.getPermission(Permission.READ, true).setExclusiveGroup(true);
grant(config, meta, Permission.READ, admin, owners);
grant(config, meta, cr, -2, 2, admin, owners);

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

@@ -2727,7 +2727,7 @@ public class ChangeIT extends AbstractDaemonTest {
TestRepository<InMemoryRepository> userTestRepo = cloneProject(p, user);
// Block default permission
block(p, "refs/*", Permission.ADD_PATCH_SET, REGISTERED_USERS);
block(p, "refs/for/*", Permission.ADD_PATCH_SET, REGISTERED_USERS);
// Create change as admin
PushOneCommit push = pushFactory.create(db, admin.getIdent(), adminTestRepo);
@@ -2771,7 +2771,7 @@ public class ChangeIT extends AbstractDaemonTest {
TestRepository<?> adminTestRepo = cloneProject(project, admin);
// Block default permission
block(p, "refs/*", Permission.ADD_PATCH_SET, REGISTERED_USERS);
block(p, "refs/for/*", Permission.ADD_PATCH_SET, REGISTERED_USERS);
// Create change as admin
PushOneCommit push = pushFactory.create(db, admin.getIdent(), adminTestRepo);
@@ -3407,7 +3407,7 @@ public class ChangeIT extends AbstractDaemonTest {
Project.NameKey p = createProject("addPatchSetEdit");
TestRepository<InMemoryRepository> userTestRepo = cloneProject(p, user);
// Block default permission
block(p, "refs/*", Permission.ADD_PATCH_SET, REGISTERED_USERS);
block(p, "refs/for/*", Permission.ADD_PATCH_SET, REGISTERED_USERS);
// Create change as user
PushOneCommit push = pushFactory.create(db, user.getIdent(), userTestRepo);
PushOneCommit.Result r = push.to("refs/for/master");

2
javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java
View File

@@ -694,7 +694,7 @@ public class ChangeEditIT extends AbstractDaemonTest {
TestRepository<InMemoryRepository> userTestRepo = cloneProject(p, user);
// Block default permission
block(p, "refs/*", Permission.ADD_PATCH_SET, REGISTERED_USERS);
block(p, "refs/for/*", Permission.ADD_PATCH_SET, REGISTERED_USERS);
// Create change as user
PushOneCommit push = pushFactory.create(db, user.getIdent(), userTestRepo);

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

@@ -129,11 +129,11 @@ public class RefControlTest {
}
private void assertCanSubmit(String ref, ProjectControl u) {
assertThat(u.controlForRef(ref).canSubmit(false, false)).named("can submit " + ref).isTrue();
assertThat(u.controlForRef(ref).canSubmit(false)).named("can submit " + ref).isTrue();
}
private void assertCannotSubmit(String ref, ProjectControl u) {
assertThat(u.controlForRef(ref).canSubmit(false, false)).named("can submit " + ref).isFalse();
assertThat(u.controlForRef(ref).canSubmit(false)).named("can submit " + ref).isFalse();
}
private void assertCanUpload(ProjectControl u) {

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

@@ -614,125 +614,6 @@ public class ProjectConfigTest extends GerritBaseTests {
.isEqualTo(Lists.newArrayList(new PermissionRule(developers)));
}
@Test
public void readConfigSubmitRefsForStarIsMigrated() throws Exception {
RevCommit rev =
tr.commit()
.add("groups", group(developers))
.add("project.config", "[access \"refs/for/*\"]\n" + " submit = 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.SUBMIT, false)).isNotNull();
PermissionRule rule = new PermissionRule(developers);
rule.setForce(true);
assertThat(as.getPermission(Permission.SUBMIT, false).getRules())
.isEqualTo(Lists.newArrayList(rule));
}
@Test
public void readConfigSubmitRefsStarIsNotMigrated() throws Exception {
RevCommit rev =
tr.commit()
.add("groups", group(developers))
.add("project.config", "[access \"refs/*\"]\n" + " submit = group Developers\n")
.create();
ProjectConfig cfg = read(rev);
AccessSection as = cfg.getAccessSection("refs/*");
assertThat(as).isNotNull();
PermissionRule rule = new PermissionRule(developers);
assertThat(as.getPermission(Permission.SUBMIT, false).getRules())
.isEqualTo(Lists.newArrayList(rule));
}
@Test
public void readConfigSubmitRefsForStarIsMigratedIntoExistingPermission() throws Exception {
RevCommit rev =
tr.commit()
.add("groups", group(developers))
.add(
"project.config",
"[access \"refs/*\"]\n"
+ " submit = group Developers\n"
+ "[access \"refs/for/*\"]\n"
+ " submit = group Developers\n")
.create();
ProjectConfig cfg = read(rev);
AccessSection as = cfg.getAccessSection("refs/*");
assertThat(as).isNotNull();
PermissionRule rule = new PermissionRule(developers);
rule.setForce(true);
assertThat(as.getPermission(Permission.SUBMIT, false).getRules())
.isEqualTo(Lists.newArrayList(rule));
}
@Test
public void readConfigAddPatchSetRefsForStarIsMigrated() throws Exception {
RevCommit rev =
tr.commit()
.add("groups", group(developers))
.add(
"project.config",
"[access \"refs/for/*\"]\n" + " addPatchSet = group Developers\n")
.create();
ProjectConfig cfg = read(rev);
AccessSection as = cfg.getAccessSection("refs/for/*");
assertThat(as).isNull();
as = cfg.getAccessSection("refs/*");
assertThat(as).isNotNull();
PermissionRule rule = new PermissionRule(developers);
assertThat(as.getPermission(Permission.ADD_PATCH_SET, false).getRules())
.isEqualTo(Lists.newArrayList(rule));
}
@Test
public void readConfigPushMergeRefsForStarIsMigrated() throws Exception {
RevCommit rev =
tr.commit()
.add("groups", group(developers))
.add("project.config", "[access \"refs/for/*\"]\n" + " pushMerge = group Developers\n")
.create();
ProjectConfig cfg = read(rev);
AccessSection as = cfg.getAccessSection("refs/for/*");
assertThat(as).isNull();
as = cfg.getAccessSection("refs/*");
assertThat(as).isNotNull();
PermissionRule rule = new PermissionRule(developers);
assertThat(as.getPermission(Permission.PUSH_MERGE, false).getRules())
.isEqualTo(Lists.newArrayList(rule));
}
@Test
public void readConfigMultiplePermissionsOnRefsForStarIsMigrated() throws Exception {
RevCommit rev =
tr.commit()
.add("groups", group(developers))
.add(
"project.config",
"[access \"refs/for/*\"]\n"
+ " pushMerge = group Developers\n"
+ " addPatchSet = group Developers\n")
.create();
ProjectConfig cfg = read(rev);
AccessSection as = cfg.getAccessSection("refs/for/*");
assertThat(as).isNull();
as = cfg.getAccessSection("refs/*");
assertThat(as).isNotNull();
assertThat(as.getPermission(Permission.PUSH_MERGE, false).getRules())
.isEqualTo(Lists.newArrayList(new PermissionRule(developers)));
assertThat(as.getPermission(Permission.ADD_PATCH_SET, false).getRules())
.isEqualTo(Lists.newArrayList(new PermissionRule(developers)));
}
@Test
public void readCommentLinkMatchButNoHtmlOrLink() throws Exception {
RevCommit rev =

16
polygerrit-ui/app/elements/admin/gr-rule-editor/gr-rule-editor.js
View File

@@ -61,17 +61,6 @@
},
];
const FORCE_SUBMIT_OPTIONS = [
{
name: 'No Submit on Push',
value: false,
},
{
name: 'Submit on Push',
value: true,
},
];
Polymer({
is: 'gr-rule-editor',
@@ -130,8 +119,7 @@
_computeForce(permission) {
return this.permissionValues.push.id === permission ||
this.permissionValues.editTopicName.id === permission ||
this.permissionValues.submit.id === permission;
this.permissionValues.editTopicName.id === permission;
},
_computeForceClass(permission) {
@@ -173,8 +161,6 @@
return FORCE_PUSH_OPTIONS;
} else if (permission === this.permissionValues.editTopicName.id) {
return FORCE_EDIT_OPTIONS;
} else if (permission === this.permissionValues.submit.id) {
return FORCE_SUBMIT_OPTIONS;
}
return [];
},

19
polygerrit-ui/app/elements/admin/gr-rule-editor/gr-rule-editor_test.html
View File

@@ -71,16 +71,6 @@ limitations under the License.
value: true,
},
];
const FORCE_SUBMIT_OPTIONS = [
{
name: 'No Submit on Push',
value: false,
},
{
name: 'Submit on Push',
value: true,
},
];
let permission = 'push';
assert.isTrue(element._computeForce(permission));
assert.equal(element._computeForceClass(permission), 'force');
@@ -92,11 +82,6 @@ limitations under the License.
assert.deepEqual(element._computeForceOptions(permission),
FORCE_EDIT_OPTIONS);
permission = 'submit';
assert.isTrue(element._computeForce(permission));
assert.equal(element._computeForceClass(permission), 'force');
assert.deepEqual(element._computeForceOptions(permission),
FORCE_SUBMIT_OPTIONS);
permission = 'editHashtags';
assert.isFalse(element._computeForce(permission));
assert.equal(element._computeForceClass(permission), '');
assert.deepEqual(element._computeForceOptions(permission), []);
@@ -139,7 +124,7 @@ limitations under the License.
{action: 'ALLOW', force: false});
permission = 'submit';
assert.deepEqual(element._getDefaultRuleValues(permission, label),
{action: 'ALLOW', force: false});
{action: 'ALLOW'});
});
test('_setDefaultRuleValues', () => {
@@ -225,7 +210,7 @@ limitations under the License.
assert.equal(element.$.action.bindValue, element.rule.value.action);
assert.isNotOk(Polymer.dom(element.root).querySelector('#labelMin'));
assert.isNotOk(Polymer.dom(element.root).querySelector('#labelMax'));
assert.isTrue(element.$.force.classList.contains('force'));
assert.isFalse(element.$.force.classList.contains('force'));
});
test('modify and cancel restores original values', () => {