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>
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AccessSectionEditor.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AccessSectionEditor.java
index e518d26..1b946cd 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AccessSectionEditor.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AccessSectionEditor.java
@@ -169,7 +169,7 @@
@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 @@
}
}
+ 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;
}
diff --git a/java/com/google/gerrit/common/data/AccessSection.java b/java/com/google/gerrit/common/data/AccessSection.java
index b525f4d..ce47881 100644
--- a/java/com/google/gerrit/common/data/AccessSection.java
+++ b/java/com/google/gerrit/common/data/AccessSection.java
@@ -34,11 +34,12 @@
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 @@
@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 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();
diff --git a/java/com/google/gerrit/server/restapi/project/SetAccessUtil.java b/java/com/google/gerrit/server/restapi/project/SetAccessUtil.java
index 422c749..c5c8cf0 100644
--- a/java/com/google/gerrit/server/restapi/project/SetAccessUtil.java
+++ b/java/com/google/gerrit/server/restapi/project/SetAccessUtil.java
@@ -115,7 +115,7 @@
}
p.add(r);
}
- accessSection.getPermissions().add(p);
+ accessSection.addPermission(p);
}
sections.add(accessSection);
}
diff --git a/javatests/com/google/gerrit/common/data/AccessSectionTest.java b/javatests/com/google/gerrit/common/data/AccessSectionTest.java
index 61ee6c9..ecdb3c8 100644
--- a/javatests/com/google/gerrit/common/data/AccessSectionTest.java
+++ b/javatests/com/google/gerrit/common/data/AccessSectionTest.java
@@ -127,7 +127,7 @@
}
@Test
- public void cannotAddPermissionByModifyingList() {
+ public void cannotAddPermissionByModifyingListThatWasProvidedToAccessSection() {
Permission abandonPermission = new Permission(Permission.ABANDON);
Permission rebasePermission = new Permission(Permission.REBASE);
@@ -143,6 +143,21 @@
}
@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);
Permission rebasePermission = new Permission(Permission.REBASE);
