AccessSection.getPermissions: Return defensive copy of list

If AccessSection directly returns its modifyable list, callers can
modify the list outside of AccessSection. This is bad because this way
it is possible to violate assumptions of the AccessSection class. E.g.
AccessSection makes sure that the permission list cannot contain
duplicate permissions. Having duplicate permissions in the permission
list can lead to severe problems. E.g. duplicate permissions on an
access section of the All-Projects project make the permissions of the
All-Project project unreadable and Gerrit effectively stops working.
This is because ProjectState#getLocalAccessSections() filters out some
permissions on the All-Projects project by getting the list of
permissions from AccessSection and setting the filtered list back on
AccessSection. If the list that was retrieved from AccessSection
contained duplicate permissions, setting back the list failed with
IllegalArgumentException since
AccessSection#setPermissions(List<Permission>) doesn't allow duplicate
permissions.

A similar issue was already fixed by change I5222cd9174 and change
Ief24c6e82f.

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

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

gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AccessSectionEditor.java

@@ -169,7 +169,7 @@ public class AccessSectionEditor extends Composite
 
   @Override
   public void setValue(AccessSection value) {
-    Collections.sort(value.getPermissions());
+    sortPermissions(value);
 
     this.value = value;
     this.readOnly = !editing || !(projectAccess.isOwnerOf(value) || projectAccess.canUpload());
@@ -204,6 +204,12 @@ public class AccessSectionEditor extends Composite
     }
   }
 
+  private void sortPermissions(AccessSection accessSection) {
+    List<Permission> permissionList = new ArrayList<>(accessSection.getPermissions());
+    Collections.sort(permissionList);
+    accessSection.setPermissions(permissionList);
+  }
+
   void setEditing(boolean editing) {
     this.editing = editing;
   }

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

@@ -34,11 +34,12 @@ public class AccessSection extends RefConfigSection implements Comparable<Access
     super(refPattern);
   }
 
+  // TODO(ekempin): Make this method return an ImmutableList once the GWT UI is gone.
   public List<Permission> getPermissions() {
     if (permissions == null) {
-      permissions = new ArrayList<>();
+      return new ArrayList<>();
     }
-    return permissions;
+    return new ArrayList<>(permissions);
   }
 
   public void setPermissions(List<Permission> list) {
@@ -59,13 +60,19 @@ public class AccessSection extends RefConfigSection implements Comparable<Access
 
   @Nullable
   public Permission getPermission(String name, boolean create) {
-    for (Permission p : getPermissions()) {
-      if (p.getName().equalsIgnoreCase(name)) {
-        return p;
+    if (permissions != null) {
+      for (Permission p : permissions) {
+        if (p.getName().equalsIgnoreCase(name)) {
+          return p;
+        }
       }
     }
 
     if (create) {
+      if (permissions == null) {
+        permissions = new ArrayList<>();
+      }
+
       Permission p = new Permission(name);
       permissions.add(p);
       return p;
@@ -75,7 +82,10 @@ public class AccessSection extends RefConfigSection implements Comparable<Access
   }
 
   public void addPermission(Permission permission) {
-    List<Permission> permissions = getPermissions();
+    if (permissions == null) {
+      permissions = new ArrayList<>();
+    }
+
     for (Permission p : permissions) {
       if (p.getName().equalsIgnoreCase(permission.getName())) {
         throw new IllegalArgumentException();

java/com/google/gerrit/server/restapi/project/SetAccessUtil.java

@@ -115,7 +115,7 @@ public class SetAccessUtil {
           }
           p.add(r);
         }
-        accessSection.getPermissions().add(p);
+        accessSection.addPermission(p);
       }
       sections.add(accessSection);
     }

javatests/com/google/gerrit/common/data/AccessSectionTest.java

@@ -127,7 +127,7 @@ public class AccessSectionTest {
   }
 
   @Test
-  public void cannotAddPermissionByModifyingList() {
+  public void cannotAddPermissionByModifyingListThatWasProvidedToAccessSection() {
     Permission abandonPermission = new Permission(Permission.ABANDON);
     Permission rebasePermission = new Permission(Permission.REBASE);
 
@@ -142,6 +142,21 @@ public class AccessSectionTest {
     assertThat(accessSection.getPermission(Permission.SUBMIT)).isNull();
   }
 
+  @Test
+  public void cannotAddPermissionByModifyingListThatWasRetrievedFromAccessSection() {
+    Permission submitPermission = new Permission(Permission.SUBMIT);
+    accessSection.getPermissions().add(submitPermission);
+    assertThat(accessSection.getPermission(Permission.SUBMIT)).isNull();
+
+    List<Permission> permissions = new ArrayList<>();
+    permissions.add(new Permission(Permission.ABANDON));
+    permissions.add(new Permission(Permission.REBASE));
+    accessSection.setPermissions(permissions);
+    assertThat(accessSection.getPermission(Permission.SUBMIT)).isNull();
+    accessSection.getPermissions().add(submitPermission);
+    assertThat(accessSection.getPermission(Permission.SUBMIT)).isNull();
+  }
+
   @Test
   public void removePermission() {
     Permission abandonPermission = new Permission(Permission.ABANDON);