opendev/gerrit
Code Issues Proposed changes

Change schema migration for revert permission to grant on refs/*

Browse Source 
Currently, we are granting the newly created "revert" permission
introduced in If5180bc98 on refs/heads/*. This is not good for some
of our users as not everyone works only on those branches. Also, for
all of our users this ref doesn't include refs/meta/config.

This change fixes the migration by deleting the permission on
refs/heads/* for Registered Users and moving it to refs/*

This will take effect only if admins haven't touched the permission.
Otherwise, we'd rather not change their preferences.

Also, new All-Projects will be created with the permission on refs/*
by default.

Change-Id: I487f5715dba7b03d7ee365b069bd4a36873584e7
This commit is contained in:
Gal Paikin
2020-05-29 10:36:39 +02:00
parent 7d44589f47
commit 73b5183442
5 changed files with 105 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files

11
java/com/google/gerrit/server/schema/AclUtil.java
View File

@@ -105,4 +105,15 @@ public class AclUtil {
public static PermissionRule rule(ProjectConfig config, GroupReference group) {
return new PermissionRule(config.resolve(group));
}
public static void remove(
ProjectConfig config, AccessSection section, String permission, GroupReference... groupList) {
Permission p = section.getPermission(permission, true);
for (GroupReference group : groupList) {
if (group != null) {
PermissionRule r = rule(config, group);
p.remove(r);
}
}
}
}

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

@@ -174,11 +174,12 @@ public class AllProjectsCreator {
AccessSection heads, LabelType codeReviewLabel, ProjectConfig config) {
AccessSection refsFor = config.getAccessSection("refs/for/*", true);
AccessSection magic = config.getAccessSection("refs/for/" + AccessSection.ALL, true);
AccessSection all = config.getAccessSection("refs/*", true);
grant(config, refsFor, Permission.ADD_PATCH_SET, registered);
grant(config, heads, codeReviewLabel, -1, 1, registered);
grant(config, heads, Permission.FORGE_AUTHOR, registered);
grant(config, heads, Permission.REVERT, registered);
grant(config, all, Permission.REVERT, registered);
grant(config, magic, Permission.PUSH, registered);
grant(config, magic, Permission.PUSH_MERGE, registered);
}

23
java/com/google/gerrit/server/schema/GrantRevertPermission.java
View File

@@ -16,6 +16,7 @@ package com.google.gerrit.server.schema;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static com.google.gerrit.server.schema.AclUtil.grant;
import static com.google.gerrit.server.schema.AclUtil.remove;
import com.google.gerrit.common.data.AccessSection;
import com.google.gerrit.common.data.GroupReference;
@@ -64,12 +65,26 @@ public class GrantRevertPermission {
ProjectConfig projectConfig = projectConfigFactory.read(md);
AccessSection heads = projectConfig.getAccessSection(AccessSection.HEADS, true);
Permission permission = heads.getPermission(Permission.REVERT);
if (permission != null && permission.getRule(registeredUsers) != null) {
// permission already exists, don't do anything.
Permission permissionOnRefsHeads = heads.getPermission(Permission.REVERT);
if (permissionOnRefsHeads != null) {
if (permissionOnRefsHeads.getRule(registeredUsers) == null
|| permissionOnRefsHeads.getRules().size() > 1) {
// If admins already changed the permission, don't do anything.
return;
}
// permission already exists in refs/heads/*, delete it for Registered Users.
remove(projectConfig, heads, Permission.REVERT, registeredUsers);
}
AccessSection all = projectConfig.getAccessSection(AccessSection.ALL, true);
Permission permissionOnRefsStar = all.getPermission(Permission.REVERT);
if (permissionOnRefsStar != null && permissionOnRefsStar.getRule(registeredUsers) != null) {
// permission already exists in refs/*, don't do anything.
return;
}
grant(projectConfig, heads, Permission.REVERT, registeredUsers);
// If the permission doesn't exist of refs/* for Registered Users, grant it.
grant(projectConfig, all, Permission.REVERT, registeredUsers);
md.getCommitBuilder().setAuthor(serverUser);
md.getCommitBuilder().setCommitter(serverUser);

2
java/com/google/gerrit/server/schema/testing/AllProjectsCreatorTestUtil.java
View File

@@ -55,6 +55,7 @@ public class AllProjectsCreatorTestUtil {
"[access \"refs/*\"]",
" read = group Administrators",
" read = group Anonymous Users",
" revert = group Registered Users",
"[access \"refs/for/*\"]",
" addPatchSet = group Registered Users",
"[access \"refs/for/refs/*\"]",
@@ -75,7 +76,6 @@ public class AllProjectsCreatorTestUtil {
" push = group Project Owners",
" submit = group Administrators",
" submit = group Project Owners",
" revert = group Registered Users",
"[access \"refs/meta/config\"]",
" exclusiveGroupPermissions = read",
" create = group Administrators",

75
javatests/com/google/gerrit/acceptance/rest/project/AccessIT.java
View File

@@ -17,7 +17,9 @@ import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth8.assertThat;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allow;
import static com.google.gerrit.extensions.client.ListChangesOption.MESSAGES;
import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static com.google.gerrit.server.schema.AclUtil.grant;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import static com.google.gerrit.truth.ConfigSubject.assertThat;
import static com.google.gerrit.truth.MapSubject.assertThatMap;
@@ -31,6 +33,7 @@ import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.common.data.AccessSection;
import com.google.gerrit.common.data.GlobalCapability;
import com.google.gerrit.common.data.GroupReference;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.RefNames;
@@ -92,7 +95,7 @@ public class AccessIT extends AbstractDaemonTest {
@Test
public void grantRevertPermission() throws Exception {
String ref = "refs/heads/*";
String ref = "refs/*";
String groupId = "global:Registered-Users";
grantRevertPermission.execute(newProjectName);
@@ -107,6 +110,72 @@ public class AccessIT extends AbstractDaemonTest {
assertThat(permissionRuleInfo.action).isEqualTo(PermissionRuleInfo.Action.ALLOW);
}
@Test
public void grantRevertPermissionByOnNewRefAndDeletingOnOldRef() throws Exception {
String refsHeads = "refs/heads/*";
String refsStar = "refs/*";
String groupId = "global:Registered-Users";
GroupReference registeredUsers = systemGroupBackend.getGroup(REGISTERED_USERS);
try (Repository repo = repoManager.openRepository(newProjectName)) {
MetaDataUpdate md = new MetaDataUpdate(GitReferenceUpdated.DISABLED, newProjectName, repo);
ProjectConfig projectConfig = projectConfigFactory.read(md);
AccessSection heads = projectConfig.getAccessSection(AccessSection.HEADS, true);
grant(projectConfig, heads, Permission.REVERT, registeredUsers);
md.getCommitBuilder().setAuthor(admin.newIdent());
md.getCommitBuilder().setCommitter(admin.newIdent());
md.setMessage("Add revert permission for all registered users\n");
projectConfig.commit(md);
}
grantRevertPermission.execute(newProjectName);
ProjectAccessInfo info = pApi().access();
// Revert permission is removed on refs/heads/*.
assertThat(info.local.containsKey(refsHeads)).isTrue();
AccessSectionInfo accessSectionInfo = info.local.get(refsHeads);
assertThat(accessSectionInfo.permissions.containsKey(Permission.REVERT)).isFalse();
// new permission is added on refs/* with Registered-Users.
assertThat(info.local.containsKey(refsStar)).isTrue();
accessSectionInfo = info.local.get(refsStar);
assertThat(accessSectionInfo.permissions.containsKey(Permission.REVERT)).isTrue();
PermissionInfo permissionInfo = accessSectionInfo.permissions.get(Permission.REVERT);
assertThat(permissionInfo.rules.containsKey(groupId)).isTrue();
PermissionRuleInfo permissionRuleInfo = permissionInfo.rules.get(groupId);
assertThat(permissionRuleInfo.action).isEqualTo(PermissionRuleInfo.Action.ALLOW);
}
@Test
public void grantRevertPermissionDoesntDeleteAdminsPreferences() throws Exception {
String refsHeads = "refs/heads/*";
GroupReference registeredUsers = systemGroupBackend.getGroup(REGISTERED_USERS);
GroupReference otherGroup = systemGroupBackend.getGroup(ANONYMOUS_USERS);
String groupId = "global:Registered-Users";
String otherGroupId = "global:Anonymous-Users";
try (Repository repo = repoManager.openRepository(newProjectName)) {
MetaDataUpdate md = new MetaDataUpdate(GitReferenceUpdated.DISABLED, newProjectName, repo);
ProjectConfig projectConfig = projectConfigFactory.read(md);
AccessSection heads = projectConfig.getAccessSection(AccessSection.HEADS, true);
grant(projectConfig, heads, Permission.REVERT, registeredUsers);
grant(projectConfig, heads, Permission.REVERT, otherGroup);
md.getCommitBuilder().setAuthor(admin.newIdent());
md.getCommitBuilder().setCommitter(admin.newIdent());
md.setMessage("Add revert permission for all registered users\n");
projectConfig.commit(md);
}
ProjectAccessInfo expected = pApi().access();
grantRevertPermission.execute(newProjectName);
projectCache.evict(newProjectName);
ProjectAccessInfo actual = pApi().access();
// Permissions don't change
assertThat(expected.local).isEqualTo(actual.local);
}
@Test
public void grantRevertPermissionOnlyWorksOnce() throws Exception {
grantRevertPermission.execute(newProjectName);
@@ -115,9 +184,9 @@ public class AccessIT extends AbstractDaemonTest {
try (Repository repo = repoManager.openRepository(newProjectName)) {
MetaDataUpdate md = new MetaDataUpdate(GitReferenceUpdated.DISABLED, newProjectName, repo);
ProjectConfig projectConfig = projectConfigFactory.read(md);
AccessSection heads = projectConfig.getAccessSection(AccessSection.HEADS, true);
AccessSection all = projectConfig.getAccessSection(AccessSection.ALL, true);
Permission permission = heads.getPermission(Permission.REVERT);
Permission permission = all.getPermission(Permission.REVERT);
assertThat(permission.getRules()).hasSize(1);
}
}