From 942a8fe8cfc29a41080c09c04e03ab288b0e487c Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Tue, 24 Apr 2018 11:56:17 -0400 Subject: [PATCH] Revert migration from refs/for/* permissions to refs/heads/* 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: 9ff07177eae4970c1773682cbfc503b329b93e78 8392e982c0fc0f2a73397fab18a381cec050514f 66c52dae95e16c1bcce9dad4ae91bfc6c0dc04ab 642f3e07e7cb98a997e15569ae01a6952a9bce67 [1] https://gerrit-review.googlesource.com/c/gerrit/+/173513 Change-Id: I139ba7aa97a946f2c31258e7dfaa3a94ee72ff6e --- Documentation/access-control.txt | 17 ++- .../client/admin/PermissionEditor.ui.xml | 2 +- .../client/admin/PermissionRuleEditor.java | 25 ++-- .../client/admin/PermissionRuleEditor.ui.xml | 2 +- .../gerrit/common/data/PermissionRule.java | 1 - .../server/permissions/ChangeControl.java | 4 +- .../gerrit/server/permissions/RefControl.java | 13 +- .../gerrit/server/project/ProjectConfig.java | 51 ++------ .../server/schema/AllProjectsCreator.java | 7 +- .../acceptance/api/change/ChangeIT.java | 6 +- .../gerrit/acceptance/edit/ChangeEditIT.java | 2 +- .../server/permissions/RefControlTest.java | 4 +- .../server/project/ProjectConfigTest.java | 119 ------------------ .../admin/gr-rule-editor/gr-rule-editor.js | 16 +-- .../gr-rule-editor/gr-rule-editor_test.html | 19 +-- 15 files changed, 54 insertions(+), 234 deletions(-) diff --git a/Documentation/access-control.txt b/Documentation/access-control.txt index bff4094f89..55049de50c 100644 --- a/Documentation/access-control.txt +++ b/Documentation/access-control.txt @@ -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/` +(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/*' 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 8d251624af..00c41dc09f 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: 9em; + width: 7em; 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 f8f33d0fec..16dd1679c3 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,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); 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 aa38d279f2..644fef4349 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: 9em; + width: 7em; font-size: 80%; } diff --git a/java/com/google/gerrit/common/data/PermissionRule.java b/java/com/google/gerrit/common/data/PermissionRule.java index e6673d83b1..c50af5c78a 100644 --- a/java/com/google/gerrit/common/data/PermissionRule.java +++ b/java/com/google/gerrit/common/data/PermissionRule.java @@ -15,7 +15,6 @@ 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 529586e09f..990c318f21 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(), 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: diff --git a/java/com/google/gerrit/server/permissions/RefControl.java b/java/com/google/gerrit/server/permissions/RefControl.java index bf3a1336a5..6a581016e4 100644 --- a/java/com/google/gerrit/server/permissions/RefControl.java +++ b/java/com/google/gerrit/server/permissions/RefControl.java @@ -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); diff --git a/java/com/google/gerrit/server/project/ProjectConfig.java b/java/com/google/gerrit/server/project/ProjectConfig.java index 55140d40a4..d2fd7cef7d 100644 --- a/java/com/google/gerrit/server/project/ProjectConfig.java +++ b/java/com/google/gerrit/server/project/ProjectConfig.java @@ -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; } diff --git a/java/com/google/gerrit/server/schema/AllProjectsCreator.java b/java/com/google/gerrit/server/schema/AllProjectsCreator.java index 8c5cabd856..fe4b2758a7 100644 --- a/java/com/google/gerrit/server/schema/AllProjectsCreator.java +++ b/java/com/google/gerrit/server/schema/AllProjectsCreator.java @@ -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); diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java index 4d22ada7ee..5f19d50002 100644 --- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java +++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java @@ -2727,7 +2727,7 @@ public class ChangeIT extends AbstractDaemonTest { TestRepository 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 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"); diff --git a/javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java b/javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java index bd3daa387c..91a12788c6 100644 --- a/javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java +++ b/javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java @@ -694,7 +694,7 @@ public class ChangeEditIT extends AbstractDaemonTest { TestRepository 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); diff --git a/javatests/com/google/gerrit/server/permissions/RefControlTest.java b/javatests/com/google/gerrit/server/permissions/RefControlTest.java index 3ef9a344a4..c54deded90 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, 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) { diff --git a/javatests/com/google/gerrit/server/project/ProjectConfigTest.java b/javatests/com/google/gerrit/server/project/ProjectConfigTest.java index 533bfe80c9..63338c45a8 100644 --- a/javatests/com/google/gerrit/server/project/ProjectConfigTest.java +++ b/javatests/com/google/gerrit/server/project/ProjectConfigTest.java @@ -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 = 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 503811dbf1..4af4952c8d 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 @@ -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 []; }, 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 666a8f6690..5b6f947f0c 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,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', () => {