From d3b8c32d8107b80ee819453bcddbd0a98330249a Mon Sep 17 00:00:00 2001
From: Gal Paikin <paiking@google.com>
Date: Fri, 8 May 2020 12:42:51 +0200
Subject: [PATCH] GrantRevertPermission only adds the permisions once

Until now, if the permission somehow already existed, it will add
it another time.
This change ensures that it only adds the permission once.

Also, this change adds a minor reformat to the other test.

Change-Id: If68f0def0fe042b1a3f669b7118e8138aa29e7fb
(cherry picked from commit 5164b2cb5a39223a21eb5dc5e1dbf451e1a832f4)
---
 .../server/schema/GrantRevertPermission.java  |  9 +++++++-
 .../acceptance/rest/project/AccessIT.java     | 23 ++++++++++++++++---
 2 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/java/com/google/gerrit/server/schema/GrantRevertPermission.java b/java/com/google/gerrit/server/schema/GrantRevertPermission.java
index ffc2b81c32..d6228ce9d9 100644
--- a/java/com/google/gerrit/server/schema/GrantRevertPermission.java
+++ b/java/com/google/gerrit/server/schema/GrantRevertPermission.java
@@ -18,6 +18,7 @@ import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS
 import static com.google.gerrit.server.schema.AclUtil.grant;
 
 import com.google.gerrit.common.data.AccessSection;
+import com.google.gerrit.common.data.GroupReference;
 import com.google.gerrit.common.data.Permission;
 import com.google.gerrit.entities.Project;
 import com.google.gerrit.server.GerritPersonIdent;
@@ -57,12 +58,18 @@ public class GrantRevertPermission {
   }
 
   public void execute(Project.NameKey projectName) throws IOException, ConfigInvalidException {
+    GroupReference registeredUsers = systemGroupBackend.getGroup(REGISTERED_USERS);
     try (Repository repo = repoManager.openRepository(projectName)) {
       MetaDataUpdate md = new MetaDataUpdate(GitReferenceUpdated.DISABLED, projectName, repo);
       ProjectConfig projectConfig = projectConfigFactory.read(md);
       AccessSection heads = projectConfig.getAccessSection(AccessSection.HEADS, true);
 
-      grant(projectConfig, heads, Permission.REVERT, systemGroupBackend.getGroup(REGISTERED_USERS));
+      Permission permission = heads.getPermission(Permission.REVERT);
+      if (permission != null && permission.getRule(registeredUsers) != null) {
+        // permission already exists, don't do anything.
+        return;
+      }
+      grant(projectConfig, heads, Permission.REVERT, registeredUsers);
 
       md.getCommitBuilder().setAuthor(serverUser);
       md.getCommitBuilder().setCommitter(serverUser);
diff --git a/javatests/com/google/gerrit/acceptance/rest/project/AccessIT.java b/javatests/com/google/gerrit/acceptance/rest/project/AccessIT.java
index f2ebad080a..fcf0e6d08d 100644
--- a/javatests/com/google/gerrit/acceptance/rest/project/AccessIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/project/AccessIT.java
@@ -52,6 +52,8 @@ import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.webui.FileHistoryWebLink;
 import com.google.gerrit.server.config.AllProjectsNameProvider;
+import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
+import com.google.gerrit.server.git.meta.MetaDataUpdate;
 import com.google.gerrit.server.group.SystemGroupBackend;
 import com.google.gerrit.server.project.ProjectConfig;
 import com.google.gerrit.server.schema.GrantRevertPermission;
@@ -91,20 +93,35 @@ public class AccessIT extends AbstractDaemonTest {
   @Test
   public void grantRevertPermission() throws Exception {
     String ref = "refs/heads/*";
-    String permissionName = "revert";
     String groupId = "global:Registered-Users";
 
     grantRevertPermission.execute(newProjectName);
+
     ProjectAccessInfo info = pApi().access();
     assertThat(info.local.containsKey(ref)).isTrue();
     AccessSectionInfo accessSectionInfo = info.local.get(ref);
-    assertThat(accessSectionInfo.permissions.containsKey(permissionName)).isTrue();
-    PermissionInfo permissionInfo = accessSectionInfo.permissions.get(permissionName);
+    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 grantRevertPermissionOnlyWorksOnce() throws Exception {
+    grantRevertPermission.execute(newProjectName);
+    grantRevertPermission.execute(newProjectName);
+
+    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);
+
+      Permission permission = heads.getPermission(Permission.REVERT);
+      assertThat(permission.getRules()).hasSize(1);
+    }
+  }
+
   @Test
   public void getDefaultInheritance() throws Exception {
     String inheritedName = pApi().access().inheritsFrom.name;