diff --git a/Documentation/access-control.txt b/Documentation/access-control.txt index 55049de50c..ff17e12ca6 100644 --- a/Documentation/access-control.txt +++ b/Documentation/access-control.txt @@ -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/` -(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 diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/PermissionEditor.ui.xml b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/PermissionEditor.ui.xml index 00c41dc09f..8d251624af 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/PermissionEditor.ui.xml +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/PermissionEditor.ui.xml @@ -58,7 +58,7 @@ limitations under the License. position: absolute; top: 0; right: 36px; - width: 7em; + width: 9em; font-size: 80%; } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/PermissionRuleEditor.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/PermissionRuleEditor.java index 16dd1679c3..f8f33d0fec 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/PermissionRuleEditor.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/PermissionRuleEditor.java @@ -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); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/PermissionRuleEditor.ui.xml b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/PermissionRuleEditor.ui.xml index 644fef4349..aa38d279f2 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/PermissionRuleEditor.ui.xml +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/PermissionRuleEditor.ui.xml @@ -55,7 +55,7 @@ limitations under the License. position: absolute; top: 0; right: 36px; - width: 7em; + width: 9em; font-size: 80%; } diff --git a/java/com/google/gerrit/common/data/PermissionRule.java b/java/com/google/gerrit/common/data/PermissionRule.java index c50af5c78a..e6673d83b1 100644 --- a/java/com/google/gerrit/common/data/PermissionRule.java +++ b/java/com/google/gerrit/common/data/PermissionRule.java @@ -15,6 +15,7 @@ package com.google.gerrit.common.data; public class PermissionRule implements Comparable { + 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"; diff --git a/java/com/google/gerrit/server/permissions/ChangeControl.java b/java/com/google/gerrit/server/permissions/ChangeControl.java index 990c318f21..529586e09f 100644 --- a/java/com/google/gerrit/server/permissions/ChangeControl.java +++ b/java/com/google/gerrit/server/permissions/ChangeControl.java @@ -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: diff --git a/java/com/google/gerrit/server/permissions/RefControl.java b/java/com/google/gerrit/server/permissions/RefControl.java index 6a581016e4..8cb1d716d0 100644 --- a/java/com/google/gerrit/server/permissions/RefControl.java +++ b/java/com/google/gerrit/server/permissions/RefControl.java @@ -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); diff --git a/java/com/google/gerrit/server/project/ProjectConfig.java b/java/com/google/gerrit/server/project/ProjectConfig.java index d2fd7cef7d..4b13b87790 100644 --- a/java/com/google/gerrit/server/project/ProjectConfig.java +++ b/java/com/google/gerrit/server/project/ProjectConfig.java @@ -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; } diff --git a/javatests/com/google/gerrit/server/permissions/RefControlTest.java b/javatests/com/google/gerrit/server/permissions/RefControlTest.java index 7e671dfa13..4a9179e25e 100644 --- a/javatests/com/google/gerrit/server/permissions/RefControlTest.java +++ b/javatests/com/google/gerrit/server/permissions/RefControlTest.java @@ -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) { diff --git a/javatests/com/google/gerrit/server/project/ProjectConfigTest.java b/javatests/com/google/gerrit/server/project/ProjectConfigTest.java index 63338c45a8..6c10fbede4 100644 --- a/javatests/com/google/gerrit/server/project/ProjectConfigTest.java +++ b/javatests/com/google/gerrit/server/project/ProjectConfigTest.java @@ -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 = diff --git a/polygerrit-ui/app/elements/admin/gr-rule-editor/gr-rule-editor.js b/polygerrit-ui/app/elements/admin/gr-rule-editor/gr-rule-editor.js index ce018090a7..d4c5fd14ba 100644 --- a/polygerrit-ui/app/elements/admin/gr-rule-editor/gr-rule-editor.js +++ b/polygerrit-ui/app/elements/admin/gr-rule-editor/gr-rule-editor.js @@ -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 []; }, diff --git a/polygerrit-ui/app/elements/admin/gr-rule-editor/gr-rule-editor_test.html b/polygerrit-ui/app/elements/admin/gr-rule-editor/gr-rule-editor_test.html index 70f0edfd0d..3e3bcfbec4 100644 --- a/polygerrit-ui/app/elements/admin/gr-rule-editor/gr-rule-editor_test.html +++ b/polygerrit-ui/app/elements/admin/gr-rule-editor/gr-rule-editor_test.html @@ -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', () => {