opendev/gerrit
Code Issues Proposed changes

"Submit on Push" option on Submit Perm.

Browse Source 
Add "Submit on Push" option to the Submit Permission and remove
the "refs/for/"-way of allowing this.

This change introduces a "Submit on Push" option for the Submit
permission, which will enable the %submit push parameter.

Any "submit refs/for/*" permission will be  automatially migrated
when the config is loaded.

Change-Id: I6106ecf289bef541fa273d3deebead7a3a9710ac
This commit is contained in:
Gustaf Lundh
2017-04-26 15:51:50 +02:00
parent 9e63fd4ecc
commit 642f3e07e7
12 changed files with 135 additions and 22 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
Documentation/access-control.txt
View File

@@ -795,8 +795,7 @@ 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 have the Submit permission on `refs/for/<ref>`
(e.g. on `refs/for/refs/heads/master`).
the caller needs to enable the "Submit on Push" option.
Submitting to the `refs/meta/config` branch is only allowed to project
owners. Any explicit submit permissions for non-project-owners on this

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: 7em;
width: 9em;
font-size: 80%;
}

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

@@ -16,6 +16,7 @@ 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;
@@ -139,15 +140,23 @@ public class PermissionRuleEditor extends Composite
initWidget(uiBinder.createAndBindUi(this));
String name = permission.getName();
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);
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;
}
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: 7em;
width: 9em;
font-size: 80%;
}

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

@@ -15,6 +15,7 @@
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()) || refControl.canRebase())
return (isOwner() || refControl.canSubmit(isOwner(), false) || 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());
return refControl.canSubmit(isOwner(), false);
case REMOVE_REVIEWER:
case SUBMIT_AS:

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

@@ -108,7 +108,7 @@ class RefControl {
}
/** @return true if this user can submit patch sets to this ref */
boolean canSubmit(boolean isChangeOwner) {
boolean canSubmit(boolean isChangeOwner, boolean force) {
if (RefNames.REFS_CONFIG.equals(refName)) {
// Always allow project owners to submit configuration changes.
// Submitting configuration changes modifies the access control
@@ -117,7 +117,7 @@ class RefControl {
// granting of powers beyond submitting to the configuration.
return projectControl.isOwner();
}
return canPerform(Permission.SUBMIT, isChangeOwner, false);
return canPerform(Permission.SUBMIT, isChangeOwner, force);
}
/** @return true if this user can force edit topic names. */
@@ -494,7 +494,7 @@ class RefControl {
return canPerform(refPermissionName(perm));
case UPDATE_BY_SUBMIT:
return projectControl.controlForRef(MagicBranch.NEW_CHANGE + refName).canSubmit(true);
return canSubmit(true, true);
case READ_PRIVATE_CHANGES:
return canPerform(Permission.VIEW_PRIVATE_CHANGES);

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

@@ -756,6 +756,23 @@ 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;
}
return false;
}

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)).named("can submit " + ref).isTrue();
assertThat(u.controlForRef(ref).canSubmit(false, false)).named("can submit " + ref).isTrue();
}
private void assertCannotSubmit(String ref, ProjectControl u) {
assertThat(u.controlForRef(ref).canSubmit(false)).named("can submit " + ref).isFalse();
assertThat(u.controlForRef(ref).canSubmit(false, false)).named("can submit " + ref).isFalse();
}
private void assertCanUpload(ProjectControl u) {

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

@@ -614,6 +614,64 @@ 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 readCommentLinkMatchButNoHtmlOrLink() throws Exception {
RevCommit rev =

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

@@ -56,6 +56,17 @@
},
];
const FORCE_SUBMIT_OPTIONS = [
{
name: 'No Submit on Push',
value: false,
},
{
name: 'Submit on Push',
value: true,
},
];
Polymer({
is: 'gr-rule-editor',
@@ -114,7 +125,8 @@
_computeForce(permission) {
return this.permissionValues.push.id === permission ||
this.permissionValues.editTopicName.id === permission;
this.permissionValues.editTopicName.id === permission ||
this.permissionValues.submit.id === permission;
},
_computeForceClass(permission) {
@@ -156,6 +168,8 @@
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,6 +71,16 @@ 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');
@@ -82,6 +92,11 @@ 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), []);
@@ -124,7 +139,7 @@ limitations under the License.
{action: 'ALLOW', force: false});
permission = 'submit';
assert.deepEqual(element._getDefaultRuleValues(permission, label),
{action: 'ALLOW'});
{action: 'ALLOW', force: false});
});
test('_setDefaultRuleValues', () => {
@@ -210,7 +225,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.isFalse(element.$.force.classList.contains('force'));
assert.isTrue(element.$.force.classList.contains('force'));
});
test('modify and cancel restores original values', () => {