Harden DefaultApplication against modification.

Change-Id: I3342e4961096a49ad9761952d73b4a08ea0eebeb

@@ -15,6 +15,8 @@
  */
  */
 package org.onosproject.core;
 package org.onosproject.core;
 
 
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import org.onosproject.security.Permission;
 import org.onosproject.security.Permission;
 
 
 import java.net.URI;
 import java.net.URI;
@@ -76,12 +78,18 @@ public class DefaultApplication implements Application {
         this.category = checkNotNull(category, "Category cannot be null");
         this.category = checkNotNull(category, "Category cannot be null");
         this.url = url;
         this.url = url;
         this.readme = checkNotNull(readme, "Readme cannot be null");
         this.readme = checkNotNull(readme, "Readme cannot be null");
-        this.icon = icon;
+        this.icon = icon == null ? new byte[0] : icon.clone();
         this.role = checkNotNull(role, "Role cannot be null");
         this.role = checkNotNull(role, "Role cannot be null");
-        this.permissions = checkNotNull(permissions, "Permissions cannot be null");
+        this.permissions = ImmutableSet.copyOf(
+                checkNotNull(permissions, "Permissions cannot be null")
+        );
         this.featuresRepo = checkNotNull(featuresRepo, "Features repo cannot be null");
         this.featuresRepo = checkNotNull(featuresRepo, "Features repo cannot be null");
-        this.features = checkNotNull(features, "Features cannot be null");
-        this.requiredApps = checkNotNull(requiredApps, "Required apps cannot be null");
+        this.features = ImmutableList.copyOf(
+                checkNotNull(features, "Features cannot be null")
+        );
+        this.requiredApps = ImmutableList.copyOf(
+                checkNotNull(requiredApps, "Required apps cannot be null")
+        );
         checkArgument(!features.isEmpty(), "There must be at least one feature");
         checkArgument(!features.isEmpty(), "There must be at least one feature");
     }
     }
 
 
@@ -117,7 +125,7 @@ public class DefaultApplication implements Application {
 
 
     @Override
     @Override
     public byte[] icon() {
     public byte[] icon() {
-        return icon;
+        return icon.clone();
     }
     }
 
 
     @Override
     @Override

@@ -15,12 +15,22 @@
  */
  */
 package org.onosproject.core;
 package org.onosproject.core;
 
 
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.testing.EqualsTester;
 import com.google.common.testing.EqualsTester;
 import org.junit.Test;
 import org.junit.Test;
+import org.onosproject.security.AppPermission;
+import org.onosproject.security.Permission;
 
 
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
 import java.util.Optional;
 import java.util.Optional;
+import java.util.Set;
 
 
+import static org.junit.Assert.assertArrayEquals;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.assertTrue;
 import static org.onosproject.app.DefaultApplicationDescriptionTest.*;
 import static org.onosproject.app.DefaultApplicationDescriptionTest.*;
 
 
@@ -43,7 +53,7 @@ public class DefaultApplicationTest {
         assertEquals("incorrect category", CATEGORY, app.category());
         assertEquals("incorrect category", CATEGORY, app.category());
         assertEquals("incorrect URL", URL, app.url());
         assertEquals("incorrect URL", URL, app.url());
         assertEquals("incorrect readme", README, app.readme());
         assertEquals("incorrect readme", README, app.readme());
-        assertEquals("incorrect icon", ICON, app.icon());
+        assertArrayEquals("incorrect icon", ICON, app.icon());
         assertEquals("incorrect role", ROLE, app.role());
         assertEquals("incorrect role", ROLE, app.role());
         assertEquals("incorrect permissions", PERMS, app.permissions());
         assertEquals("incorrect permissions", PERMS, app.permissions());
         assertEquals("incorrect features repo", FURL, app.featuresRepo().get());
         assertEquals("incorrect features repo", FURL, app.featuresRepo().get());
@@ -69,4 +79,138 @@ public class DefaultApplicationTest {
         new EqualsTester().addEqualityGroup(a1, a2)
         new EqualsTester().addEqualityGroup(a1, a2)
                 .addEqualityGroup(a3).addEqualityGroup(a4).testEquals();
                 .addEqualityGroup(a3).addEqualityGroup(a4).testEquals();
     }
     }
+
+
+    private static final byte[] ICON_ORIG = new byte[] {1, 2, 3, 4};
+
+    @Test
+    public void immutableIcon() {
+        byte[] iconSourceData = ICON_ORIG.clone();
+
+        Application app = new DefaultApplication(APP_ID, VER, DESC, ORIGIN,
+                CATEGORY, URL, README, iconSourceData, ROLE,
+                PERMS, Optional.of(FURL), FEATURES, APPS);
+
+        // can we modify the icon after getting a reference to the app?
+        byte[] icon = app.icon();
+        assertArrayEquals("did not start with orig icon", ICON_ORIG, icon);
+
+        // now the hack
+        for (int i = 0, n = ICON_ORIG.length; i < n; i++) {
+            icon[i] = 0;
+        }
+        // if the reference to the internal array is given out, the hack
+        // will succeed and this next assertion fails
+        assertArrayEquals("no longer orig icon", ICON_ORIG, app.icon());
+
+        // what if we modify the source data?
+        for (int i = 0, n = ICON_ORIG.length; i < n; i++) {
+            iconSourceData[i] = 0;
+        }
+        // if the application just saved a reference to the given array
+        // this next assertion fails
+        assertArrayEquals("modifying source alters appicon", ICON_ORIG, app.icon());
+    }
+
+    private static final Permission PERM_W =
+            new Permission(AppPermission.class.getName(), "FLOWRULE_WRITE");
+    private static final Permission PERM_R =
+            new Permission(AppPermission.class.getName(), "FLOWRULE_READ");
+
+    private static final Permission JUNK_PERM = new Permission("foo", "bar");
+
+    private static final Set<Permission> PERMS_ORIG = ImmutableSet.of(PERM_W, PERM_R);
+    private static final Set<Permission> PERMS_UNSAFE = new HashSet<>(PERMS_ORIG);
+
+
+    @Test
+    public void immutablePermissions() {
+//        Set<Permission> p = PERMS_ORIG;
+        Set<Permission> p = PERMS_UNSAFE;
+
+        Application app = new DefaultApplication(APP_ID, VER, DESC, ORIGIN,
+                CATEGORY, URL, README, ICON, ROLE,
+                p, Optional.of(FURL), FEATURES, APPS);
+
+        Set<Permission> perms = app.permissions();
+        try {
+            perms.add(JUNK_PERM);
+        } catch (UnsupportedOperationException e) {
+            // set is immutable
+        }
+        assertTrue("no write perm", app.permissions().contains(PERM_W));
+        assertTrue("no read perm", app.permissions().contains(PERM_R));
+        assertEquals("extra perms", 2, app.permissions().size());
+
+        // DONE: review - is it sufficient to expect caller to pass in ImmutableSet ?
+        // Issue Resolved with Immutable collections used during construction.
+
+        // If we just pass in a HashSet, the contents would be modifiable by
+        // an external party. (Making the field final just means that the
+        // reference to the set can never change; the contents may still...)
+
+        // Similar reasoning can be applied to these two fields also:
+        //     List<String> features
+        //     List<String> requiredApps
+    }
+
+    private static final String FOO = "foo";
+    private static final String BAR = "bar";
+    private static final String FIFI = "fifi";
+    private static final String EVIL = "Bwahahahaha!";
+
+    private static final List<String> FEATURES_ORIG = ImmutableList.of(FOO, BAR);
+    private static final List<String> FEATURES_UNSAFE = new ArrayList<>(FEATURES_ORIG);
+
+    private static final List<String> REQ_APPS_ORIG = ImmutableList.of(FIFI);
+    private static final List<String> REQ_APPS_UNSAFE = new ArrayList<>(REQ_APPS_ORIG);
+
+    @Test
+    public void immutableFeatures() {
+//        List<String> f = FEATURES_ORIG;
+        List<String> f = FEATURES_UNSAFE;
+
+        Application app = new DefaultApplication(APP_ID, VER, DESC, ORIGIN,
+                CATEGORY, URL, README, ICON, ROLE,
+                PERMS, Optional.of(FURL), f, APPS);
+
+        List<String> features = app.features();
+        try {
+            features.add(EVIL);
+        } catch (UnsupportedOperationException e) {
+            // list is immutable
+        }
+        assertTrue("no foo feature", features.contains(FOO));
+        assertTrue("no bar feature", features.contains(BAR));
+        assertEquals("extra features!", 2, features.size());
+    }
+
+    @Test
+    public void immutableRequiredApps() {
+//        List<String> ra = REQ_APPS_ORIG;
+        List<String> ra = REQ_APPS_UNSAFE;
+
+        Application app = new DefaultApplication(APP_ID, VER, DESC, ORIGIN,
+                CATEGORY, URL, README, ICON, ROLE,
+                PERMS, Optional.of(FURL), FEATURES, ra);
+
+        List<String> reqApps = app.requiredApps();
+        try {
+            reqApps.add(EVIL);
+        } catch (UnsupportedOperationException e) {
+            // list is immutable
+        }
+        assertTrue("no fifi required app", reqApps.contains(FIFI));
+        assertEquals("extra required apps!", 1, reqApps.size());
+    }
+
+    @Test
+    public void nullIcon() {
+        Application app = new DefaultApplication(APP_ID, VER, DESC, ORIGIN,
+                CATEGORY, URL, README, null, ROLE,
+                PERMS, Optional.of(FURL), FEATURES, APPS);
+        byte[] icon = app.icon();
+        assertNotNull("null icon", icon);
+        assertEquals("unexpected size", 0, icon.length);
+    }
 }
 }
\ No newline at end of file