Permission: Don't hand out the modifyable rules list to callers

It's bad practise to give a private list member to callers and let them
do direct modifications on the list. It's better to keep the list
protected within the class. E.g. this would allow us to implement
constraints on the list.

Ideally we would return an ImmutableList from Permission.getRules() but
we cannot use ImmutableList in classes that are used by the GWT UI.

Change-Id: I32ee5e7055d99e0a7865f7c5deb0bbbd1a09e29f
Signed-off-by: Edwin Kempin <ekempin@google.com>

java/com/google/gerrit/acceptance/AbstractDaemonTest.java

@@ -1077,7 +1077,7 @@ public abstract class AbstractDaemonTest {
       ProjectConfig config = ProjectConfig.read(md);
       AccessSection s = config.getAccessSection(ref, true);
       Permission p = s.getPermission(permission, true);
-      p.getRules().clear();
+      p.clearRules();
       config.commit(md);
       projectCache.evict(config.getProject());
     }

java/com/google/gerrit/common/data/Permission.java

@@ -155,13 +155,16 @@ public class Permission implements Comparable<Permission> {
     exclusiveGroup = newExclusiveGroup;
   }
 
+  // TODO(ekempin): Make this method return an ImmutableList once the GWT UI is gone.
   public List<PermissionRule> getRules() {
-    initRules();
+    if (rules == null) {
-    return rules;
+      return new ArrayList<>();
+    }
+    return new ArrayList<>(rules);
   }
 
   public void setRules(List<PermissionRule> list) {
-    rules = list;
+    rules = new ArrayList<>(list);
   }
 
   public void add(PermissionRule rule) {
@@ -181,6 +184,12 @@ public class Permission implements Comparable<Permission> {
     }
   }
 
+  public void clearRules() {
+    if (rules != null) {
+      rules.clear();
+    }
+  }
+
   public PermissionRule getRule(GroupReference group) {
     return getRule(group, false);
   }

java/com/google/gerrit/server/project/AccountsSection.java

@@ -14,6 +14,7 @@
 
 package com.google.gerrit.server.project;
 
+import com.google.common.collect.ImmutableList;
 import com.google.gerrit.common.data.PermissionRule;
 import java.util.ArrayList;
 import java.util.List;
@@ -21,14 +22,14 @@ import java.util.List;
 public class AccountsSection {
   protected List<PermissionRule> sameGroupVisibility;
 
-  public List<PermissionRule> getSameGroupVisibility() {
+  public ImmutableList<PermissionRule> getSameGroupVisibility() {
     if (sameGroupVisibility == null) {
-      sameGroupVisibility = new ArrayList<>();
+      sameGroupVisibility = ImmutableList.of();
     }
-    return sameGroupVisibility;
+    return ImmutableList.copyOf(sameGroupVisibility);
   }
 
   public void setSameGroupVisibility(List<PermissionRule> sameGroupVisibility) {
-    this.sameGroupVisibility = sameGroupVisibility;
+    this.sameGroupVisibility = new ArrayList<>(sameGroupVisibility);
   }
 }

java/com/google/gerrit/server/project/ProjectConfig.java

@@ -737,7 +737,7 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
     }
   }
 
-  private List<PermissionRule> loadPermissionRules(
+  private ImmutableList<PermissionRule> loadPermissionRules(
       Config rc,
       String section,
       String subsection,
@@ -746,7 +746,7 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
       boolean useRange) {
     Permission perm = new Permission(varName);
     loadPermissionRules(rc, section, subsection, varName, groupsByName, perm, useRange);
-    return perm.getRules();
+    return ImmutableList.copyOf(perm.getRules());
   }
 
   private void loadPermissionRules(

javatests/com/google/gerrit/acceptance/git/PushPermissionsIT.java

@@ -336,8 +336,7 @@ public class PushPermissionsIT extends AbstractDaemonTest {
             c ->
                 cfg.getAccessSection(AccessSection.GLOBAL_CAPABILITIES, true)
                     .getPermission(c, true)
-                    .getRules()
+                    .clearRules());
-                    .clear());
   }
 
   private PushResult push(String... refSpecs) throws Exception {