From 73b5183442077ba2f2ae347d2fb2937111b8062b Mon Sep 17 00:00:00 2001 From: Gal Paikin Date: Fri, 29 May 2020 10:36:39 +0200 Subject: [PATCH] Change schema migration for revert permission to grant on refs/* 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 --- .../google/gerrit/server/schema/AclUtil.java | 11 +++ .../server/schema/AllProjectsCreator.java | 3 +- .../server/schema/GrantRevertPermission.java | 23 +++++- .../testing/AllProjectsCreatorTestUtil.java | 2 +- .../acceptance/rest/project/AccessIT.java | 75 ++++++++++++++++++- 5 files changed, 105 insertions(+), 9 deletions(-) diff --git a/java/com/google/gerrit/server/schema/AclUtil.java b/java/com/google/gerrit/server/schema/AclUtil.java index f0aafef42a..f6c3aad1f1 100644 --- a/java/com/google/gerrit/server/schema/AclUtil.java +++ b/java/com/google/gerrit/server/schema/AclUtil.java @@ -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); + } + } + } } diff --git a/java/com/google/gerrit/server/schema/AllProjectsCreator.java b/java/com/google/gerrit/server/schema/AllProjectsCreator.java index c19be5ed6f..cfa5825169 100644 --- a/java/com/google/gerrit/server/schema/AllProjectsCreator.java +++ b/java/com/google/gerrit/server/schema/AllProjectsCreator.java @@ -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); } diff --git a/java/com/google/gerrit/server/schema/GrantRevertPermission.java b/java/com/google/gerrit/server/schema/GrantRevertPermission.java index d6228ce9d9..2f890d52c9 100644 --- a/java/com/google/gerrit/server/schema/GrantRevertPermission.java +++ b/java/com/google/gerrit/server/schema/GrantRevertPermission.java @@ -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); diff --git a/java/com/google/gerrit/server/schema/testing/AllProjectsCreatorTestUtil.java b/java/com/google/gerrit/server/schema/testing/AllProjectsCreatorTestUtil.java index 606d851187..8b159bce7f 100644 --- a/java/com/google/gerrit/server/schema/testing/AllProjectsCreatorTestUtil.java +++ b/java/com/google/gerrit/server/schema/testing/AllProjectsCreatorTestUtil.java @@ -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", diff --git a/javatests/com/google/gerrit/acceptance/rest/project/AccessIT.java b/javatests/com/google/gerrit/acceptance/rest/project/AccessIT.java index fcf0e6d08d..0e23603a17 100644 --- a/javatests/com/google/gerrit/acceptance/rest/project/AccessIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/project/AccessIT.java @@ -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); } }