handle implied READ_EXTERNAL_STORAGE permissions

Having _WRITE_EXTERNAL_STORAGE_ will implied _READ_EXTERNAL_STORAGE_:
https://developer.android.com/reference/android/Manifest.permission#READ_EXTERNAL_STORAGE

closes #1702

app/src/androidTest/assets/extendedPerms.xml

@@ -68,7 +68,7 @@
             </permissions>
             <uses-permission name="android.permission.GET_ACCOUNTS" maxSdkVersion="22" />
             <uses-permission name="android.permission.READ_EXTERNAL_STORAGE" maxSdkVersion="18" />
-            <uses-permission name="android.permission.WRITE_EXTERNAL_STORAGE" maxSdkVersion="18" />
+            <uses-permission name="android.permission.WRITE_EXTERNAL_STORAGE" />
             <uses-permission name="android.permission.AUTHENTICATE_ACCOUNTS" maxSdkVersion="22" />
             <uses-permission name="android.permission.MANAGE_ACCOUNTS" maxSdkVersion="22" />
         </package>
@@ -85,7 +85,7 @@
             <targetSdkVersion>23</targetSdkVersion>
             <added>2016-06-26</added>
             <permissions>
-                org.dmfs.permission.READ_TASKS,READ_EXTERNAL_STORAGE,WRITE_CONTACTS,GET_ACCOUNTS,AUTHENTICATE_ACCOUNTS,WRITE_EXTERNAL_STORAGE,READ_CALENDAR,ACCESS_WIFI_STATE,org.dmfs.permission.WRITE_TASKS,ACCESS_NETWORK_STATE,WRITE_CALENDAR,READ_CONTACTS,READ_SYNC_SETTINGS,INTERNET,MANAGE_ACCOUNTS,WRITE_SYNC_SETTINGS
+                org.dmfs.permission.READ_TASKS,WRITE_CONTACTS,GET_ACCOUNTS,AUTHENTICATE_ACCOUNTS,WRITE_EXTERNAL_STORAGE,READ_CALENDAR,ACCESS_WIFI_STATE,org.dmfs.permission.WRITE_TASKS,ACCESS_NETWORK_STATE,WRITE_CALENDAR,READ_CONTACTS,READ_SYNC_SETTINGS,INTERNET,MANAGE_ACCOUNTS,WRITE_SYNC_SETTINGS
             </permissions>
         </package>
         <package>

app/src/androidTest/java/org/fdroid/fdroid/installer/ApkVerifierTest.java

@@ -26,13 +26,12 @@ import android.support.annotation.NonNull;
 import android.support.test.InstrumentationRegistry;
 import android.support.test.runner.AndroidJUnit4;
 import android.util.Log;
-
 import org.fdroid.fdroid.AssetUtils;
-import org.fdroid.fdroid.data.RepoXMLHandler;
 import org.fdroid.fdroid.Utils;
 import org.fdroid.fdroid.compat.FileCompatTest;
 import org.fdroid.fdroid.data.Apk;
 import org.fdroid.fdroid.data.Repo;
+import org.fdroid.fdroid.data.RepoXMLHandler;
 import org.fdroid.fdroid.mock.RepoDetails;
 import org.junit.Before;
 import org.junit.Test;
@@ -45,6 +44,7 @@ import java.io.InputStream;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.HashSet;
+import java.util.TreeSet;
 
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
@@ -103,7 +103,7 @@ public class ApkVerifierTest {
     public void testNulls() {
         assertTrue(ApkVerifier.requestedPermissionsEqual(null, null));
 
-        String[] perms = new String[] {"Blah"};
+        String[] perms = new String[]{"Blah"};
         assertFalse(ApkVerifier.requestedPermissionsEqual(perms, null));
         assertFalse(ApkVerifier.requestedPermissionsEqual(null, perms));
     }
@@ -290,7 +290,7 @@ public class ApkVerifierTest {
     public void testExtendedPerms() throws IOException,
             ApkVerifier.ApkPermissionUnequalException, ApkVerifier.ApkVerificationException {
         RepoDetails actualDetails = getFromFile(extendedPermsXml);
-        HashSet<String> expectedSet = new HashSet<>(Arrays.asList(new String[]{
+        HashSet<String> expectedSet = new HashSet<>(Arrays.asList(
                 "android.permission.ACCESS_NETWORK_STATE",
                 "android.permission.ACCESS_WIFI_STATE",
                 "android.permission.INTERNET",
@@ -301,8 +301,8 @@ public class ApkVerifierTest {
                 "android.permission.READ_CONTACTS",
                 "android.permission.WRITE_CONTACTS",
                 "android.permission.READ_CALENDAR",
-                "android.permission.WRITE_CALENDAR",
+                "android.permission.WRITE_CALENDAR"
-        }));
+        ));
         if (Build.VERSION.SDK_INT <= 18) {
             expectedSet.add("android.permission.READ_EXTERNAL_STORAGE");
             expectedSet.add("android.permission.WRITE_EXTERNAL_STORAGE");
@@ -345,6 +345,87 @@ public class ApkVerifierTest {
         apkVerifier.verifyApk();
     }
 
+    @Test
+    public void testImpliedPerms() throws IOException {
+        RepoDetails actualDetails = getFromFile(extendedPermsXml);
+        TreeSet<String> expectedSet = new TreeSet<>(Arrays.asList(
+                "android.permission.ACCESS_NETWORK_STATE",
+                "android.permission.ACCESS_WIFI_STATE",
+                "android.permission.INTERNET",
+                "android.permission.READ_CALENDAR",
+                "android.permission.READ_CONTACTS",
+                "android.permission.READ_EXTERNAL_STORAGE",
+                "android.permission.READ_SYNC_SETTINGS",
+                "android.permission.REQUEST_IGNORE_BATTERY_OPTIMIZATIONS",
+                "android.permission.WRITE_CALENDAR",
+                "android.permission.WRITE_CONTACTS",
+                "android.permission.WRITE_EXTERNAL_STORAGE",
+                "android.permission.WRITE_SYNC_SETTINGS",
+                "org.dmfs.permission.READ_TASKS",
+                "org.dmfs.permission.WRITE_TASKS"
+        ));
+        if (Build.VERSION.SDK_INT <= 22) { // maxSdkVersion="22"
+            expectedSet.addAll(Arrays.asList(
+                    "android.permission.AUTHENTICATE_ACCOUNTS",
+                    "android.permission.GET_ACCOUNTS",
+                    "android.permission.MANAGE_ACCOUNTS"
+            ));
+        }
+        Apk apk = actualDetails.apks.get(1);
+        Log.i(TAG, "APK: " + apk.apkName);
+        HashSet<String> actualSet = new HashSet<>(Arrays.asList(apk.requestedPermissions));
+        for (String permission : expectedSet) {
+            if (!actualSet.contains(permission)) {
+                Log.i(TAG, permission + " in expected but not actual! (android-"
+                        + Build.VERSION.SDK_INT + ")");
+            }
+        }
+        for (String permission : actualSet) {
+            if (!expectedSet.contains(permission)) {
+                Log.i(TAG, permission + " in actual but not expected! (android-"
+                        + Build.VERSION.SDK_INT + ")");
+            }
+        }
+        String[] expectedPermissions = expectedSet.toArray(new String[expectedSet.size()]);
+        assertTrue(ApkVerifier.requestedPermissionsEqual(expectedPermissions, apk.requestedPermissions));
+
+        expectedSet = new TreeSet<>(Arrays.asList(
+                "android.permission.ACCESS_NETWORK_STATE",
+                "android.permission.ACCESS_WIFI_STATE",
+                "android.permission.AUTHENTICATE_ACCOUNTS",
+                "android.permission.GET_ACCOUNTS",
+                "android.permission.INTERNET",
+                "android.permission.MANAGE_ACCOUNTS",
+                "android.permission.READ_CALENDAR",
+                "android.permission.READ_CONTACTS",
+                "android.permission.READ_EXTERNAL_STORAGE",
+                "android.permission.READ_SYNC_SETTINGS",
+                "android.permission.WRITE_CALENDAR",
+                "android.permission.WRITE_CONTACTS",
+                "android.permission.WRITE_EXTERNAL_STORAGE",
+                "android.permission.WRITE_SYNC_SETTINGS",
+                "org.dmfs.permission.READ_TASKS",
+                "org.dmfs.permission.WRITE_TASKS"
+        ));
+        expectedPermissions = expectedSet.toArray(new String[expectedSet.size()]);
+        apk = actualDetails.apks.get(2);
+        Log.i(TAG, "APK: " + apk.apkName);
+        actualSet = new HashSet<>(Arrays.asList(apk.requestedPermissions));
+        for (String permission : expectedSet) {
+            if (!actualSet.contains(permission)) {
+                Log.i(TAG, permission + " in expected but not actual! (android-"
+                        + Build.VERSION.SDK_INT + ")");
+            }
+        }
+        for (String permission : actualSet) {
+            if (!expectedSet.contains(permission)) {
+                Log.i(TAG, permission + " in actual but not expected! (android-"
+                        + Build.VERSION.SDK_INT + ")");
+            }
+        }
+        assertTrue(ApkVerifier.requestedPermissionsEqual(expectedPermissions, apk.requestedPermissions));
+    }
+
     @NonNull
     private RepoDetails getFromFile(File indexFile) throws IOException {
         InputStream inputStream = null;

app/src/main/java/org/fdroid/fdroid/data/Apk.java

@@ -1,5 +1,6 @@
 package org.fdroid.fdroid.data;
 
+import android.Manifest;
 import android.annotation.TargetApi;
 import android.content.ContentValues;
 import android.content.Context;
@@ -486,6 +487,16 @@ public class Apk extends ValueObject implements Comparable<Apk>, Parcelable {
         setRequestedPermissions(permissions, 23);
     }
 
+    /**
+     * Generate the set of requested permissions for the current Android version.
+     * <p>
+     * There are also a bunch of crazy rules where having one permission will imply
+     * another permission, for example, {@link Manifest.permission#WRITE_EXTERNAL_STORAGE}
+     * implies {@code Manifest.permission#READ_EXTERNAL_STORAGE}.  Many of these rules
+     * are for quite old Android versions, so they are not included here.
+     *
+     * @see Manifest.permission#READ_EXTERNAL_STORAGE
+     */
     private void setRequestedPermissions(Object[][] permissions, int minSdk) {
         HashSet<String> set = new HashSet<>();
         if (requestedPermissions != null) {
@@ -500,6 +511,9 @@ public class Apk extends ValueObject implements Comparable<Apk>, Parcelable {
                 set.add((String) versions[0]);
             }
         }
+        if (Build.VERSION.SDK_INT >= 16 && set.contains(Manifest.permission.WRITE_EXTERNAL_STORAGE)) {
+            set.add(Manifest.permission.READ_EXTERNAL_STORAGE);
+        }
         requestedPermissions = set.toArray(new String[set.size()]);
     }
 

app/src/main/java/org/fdroid/fdroid/data/RepoXMLHandler.java

@@ -19,6 +19,7 @@
 
 package org.fdroid.fdroid.data;
 
+import android.Manifest;
 import android.os.Build;
 import android.support.annotation.NonNull;
 import android.support.annotation.Nullable;
@@ -98,6 +99,10 @@ public class RepoXMLHandler extends DefaultHandler {
         if ("application".equals(localName) && curapp != null) {
             onApplicationParsed();
         } else if ("package".equals(localName) && curapk != null && curapp != null) {
+            if (Build.VERSION.SDK_INT >= 16 &&
+                    requestedPermissionsSet.contains(Manifest.permission.WRITE_EXTERNAL_STORAGE)) {
+                requestedPermissionsSet.add(Manifest.permission.READ_EXTERNAL_STORAGE);
+            }
             int size = requestedPermissionsSet.size();
             curapk.requestedPermissions = requestedPermissionsSet.toArray(new String[size]);
             requestedPermissionsSet.clear();

app/src/test/java/org/fdroid/fdroid/updater/IndexV1UpdaterTest.java

@@ -38,6 +38,8 @@ import java.io.IOException;
 import java.io.InputStream;
 import java.lang.reflect.Field;
 import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashSet;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -53,6 +55,7 @@ import static org.junit.Assert.assertArrayEquals;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
@@ -132,6 +135,14 @@ public class IndexV1UpdaterTest extends FDroidProviderTest {
         InstalledAppTestUtils.install(context, "com.waze", 1019841, "v3.9.5.4", "362488e7be5ea0689b4e97d989ae1404",
                 "cbbdb8c5dafeccd7dd7b642dde0477d3489e18ac366e3c8473d5c07e5f735a95");
         assertEquals(1, AppProvider.Helper.findInstalledAppsWithKnownVulns(context).size());
+
+        Apk apk = ApkProvider.Helper.findApkFromAnyRepo(context, "io.proto.player", 1110);
+        assertNotNull("We should find this APK", apk);
+        assertEquals("io.proto.player-1.apk", apk.apkName);
+        HashSet<String> requestedPermissions = new HashSet<>(Arrays.asList(apk.requestedPermissions));
+        assertTrue(requestedPermissions.contains(android.Manifest.permission.READ_EXTERNAL_STORAGE));
+        assertTrue(requestedPermissions.contains(android.Manifest.permission.WRITE_EXTERNAL_STORAGE));
+        assertFalse(requestedPermissions.contains(android.Manifest.permission.READ_CALENDAR));
     }
 
     @Test(expected = IndexUpdater.SigningException.class)