opendev/gerrit
Code Issues Proposed changes

Merge changes from topic "no-refs-for-permissions"

Browse Source 
* changes:
  Migrate refs/for after all sections and permissions are loaded.
  PUSH_MERGE should evaluate against refs/heads/*
  ADD_PATCH_SET should evaluate against refs/heads/*
  "Submit on Push" option on Submit Perm.
This commit is contained in:
Gustaf Lundh
2018-04-20 11:52:42 +00:00
committed by Gerrit Code Review
parent d50c8eb223 9ff07177ea
commit ce4552b323
15 changed files with 237 additions and 54 deletions
Download Patch File Download Diff File Expand all files Collapse all files

17
Documentation/access-control.txt
View File

@@ -610,10 +610,9 @@ 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. This permission needs to be
set on `refs/for/*`.
allowed to upload new patch sets for their changes.
By default, this permission is granted to `Registered Users` on `refs/for/*`,
By default, this permission is granted to `Registered Users` on `refs/*`,
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.
@@ -629,13 +628,6 @@ 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]]
@@ -795,8 +787,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
@@ -917,7 +908,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/for/refs/heads/*'
* xref:category_push_merge[`Push merge commit`] to '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: 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:

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

@@ -29,7 +29,6 @@ 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;
@@ -97,9 +96,7 @@ class RefControl {
/** @return true if this user can add a new patch set to this ref */
boolean canAddPatchSet() {
return projectControl
.controlForRef(MagicBranch.NEW_CHANGE + refName)
.canPerform(Permission.ADD_PATCH_SET);
return canPerform(Permission.ADD_PATCH_SET);
}
/** @return true if this user can rebase changes on this ref */
@@ -108,7 +105,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 +114,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. */
@@ -153,7 +150,7 @@ class RefControl {
/** @return true if this user can submit merge patch sets to this ref */
private boolean canUploadMerges() {
return projectControl.controlForRef("refs/for/" + refName).canPerform(Permission.PUSH_MERGE);
return canPerform(Permission.PUSH_MERGE);
}
/** @return true if the user can update the reference as a fast-forward. */
@@ -494,7 +491,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);

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

@@ -312,6 +312,9 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
remove(section);
} else if (section != null) {
AccessSection a = accessSections.get(section.getName());
if (a == null) {
System.out.println(a);
}
a.remove(permission);
if (a.getPermissions().isEmpty()) {
remove(a);
@@ -703,19 +706,13 @@ 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)) {
@@ -729,6 +726,20 @@ 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/");
}
@@ -756,6 +767,35 @@ 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,13 +162,11 @@ 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, refsFor, Permission.ADD_PATCH_SET, registered);
grant(config, all, Permission.ADD_PATCH_SET, registered);
if (batch != null) {
Permission priority = cap.getPermission(GlobalCapability.PRIORITY, true);
@@ -186,6 +184,7 @@ 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);
@@ -195,8 +194,6 @@ 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/for/*", Permission.ADD_PATCH_SET, REGISTERED_USERS);
block(p, "refs/*", 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/for/*", Permission.ADD_PATCH_SET, REGISTERED_USERS);
block(p, "refs/*", 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/for/*", Permission.ADD_PATCH_SET, REGISTERED_USERS);
block(p, "refs/*", 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/for/*", Permission.ADD_PATCH_SET, REGISTERED_USERS);
block(p, "refs/*", 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)).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) {

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

@@ -614,6 +614,125 @@ 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

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