Close #9830 Do no require write storage permission to downloading files on devices with Android 10 and above

Author: czlucius

components/feature/downloads/src/main/AndroidManifest.xml

@@ -8,7 +8,8 @@
     package="mozilla.components.feature.downloads">
 
     <uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE"
-        tools:ignore="ScopedStorage" />
+        tools:ignore="ScopedStorage"
+        android:maxSdkVersion="28"/>
     <uses-permission android:name="android.permission.FOREGROUND_SERVICE" />
     <uses-permission android:name="android.permission.DOWNLOAD_WITHOUT_NOTIFICATION" />
 

components/feature/downloads/src/main/java/mozilla/components/feature/downloads/manager/AndroidDownloadManager.kt

@@ -13,8 +13,10 @@ import android.content.BroadcastReceiver
 import android.content.Context
 import android.content.Intent
 import android.content.IntentFilter
+import android.os.Build
+import android.os.Build.VERSION.SDK_INT
 import android.util.LongSparseArray
-import androidx.annotation.RequiresPermission
+import androidx.annotation.VisibleForTesting
 import androidx.core.content.getSystemService
 import androidx.core.net.toUri
 import androidx.core.util.set
@@ -46,15 +48,23 @@ class AndroidDownloadManager(
     private val downloadRequests = LongSparseArray<SystemRequest>()
     private var isSubscribedReceiver = false
 
-    override val permissions = arrayOf(INTERNET, WRITE_EXTERNAL_STORAGE)
+    // Do not require WRITE_EXTERNAL_STORAGE permission on API 29 and above (using scoped storage)
+    override val permissions
+        get() = if (getSDKVersion() >= Build.VERSION_CODES.Q) {
+            arrayOf(INTERNET)
+        } else {
+            arrayOf(INTERNET, WRITE_EXTERNAL_STORAGE)
+        }
+
+    @VisibleForTesting
+    internal fun getSDKVersion() = SDK_INT
 
     /**
      * Schedules a download through the [AndroidDownloadManager].
      * @param download metadata related to the download.
      * @param cookie any additional cookie to add as part of the download request.
      * @return the id reference of the scheduled download.
      */
-    @RequiresPermission(allOf = [INTERNET, WRITE_EXTERNAL_STORAGE])
     override fun download(download: DownloadState, cookie: String): String? {
         val androidDownloadManager: SystemDownloadManager = applicationContext.getSystemService()!!
 
@@ -106,8 +116,9 @@ class AndroidDownloadManager(
     override fun onReceive(context: Context, intent: Intent) {
         val downloadID = intent.getStringExtra(EXTRA_DOWNLOAD_ID) ?: ""
         val download = store.state.downloads[downloadID]
-        val downloadStatus = intent.getSerializableExtra(AbstractFetchDownloadService.EXTRA_DOWNLOAD_STATUS)
-            as Status
+        val downloadStatus =
+            intent.getSerializableExtra(AbstractFetchDownloadService.EXTRA_DOWNLOAD_STATUS)
+                as Status
 
         if (download != null) {
             onDownloadStopped(download, downloadID, downloadStatus)

components/feature/downloads/src/main/java/mozilla/components/feature/downloads/manager/FetchDownloadManager.kt

@@ -7,14 +7,17 @@ package mozilla.components.feature.downloads.manager
 import android.Manifest.permission.FOREGROUND_SERVICE
 import android.Manifest.permission.INTERNET
 import android.Manifest.permission.WRITE_EXTERNAL_STORAGE
+import android.annotation.SuppressLint
 import android.app.DownloadManager.ACTION_DOWNLOAD_COMPLETE
 import android.app.DownloadManager.EXTRA_DOWNLOAD_ID
 import android.content.BroadcastReceiver
 import android.content.Context
 import android.content.Intent
 import android.content.IntentFilter
+import android.os.Build
 import android.os.Build.VERSION.SDK_INT
 import android.os.Build.VERSION_CODES.P
+import androidx.annotation.VisibleForTesting
 import androidx.localbroadcastmanager.content.LocalBroadcastManager
 import mozilla.components.browser.state.action.DownloadAction
 import mozilla.components.browser.state.state.content.DownloadState
@@ -41,11 +44,19 @@ class FetchDownloadManager<T : AbstractFetchDownloadService>(
 
     private var isSubscribedReceiver = false
 
-    override val permissions = if (SDK_INT >= P) {
-        arrayOf(INTERNET, WRITE_EXTERNAL_STORAGE, FOREGROUND_SERVICE)
-    } else {
-        arrayOf(INTERNET, WRITE_EXTERNAL_STORAGE)
-    }
+    // Do not require WRITE_EXTERNAL_STORAGE permission on API 29 and above (using scoped storage)
+    override val permissions
+        @SuppressLint("InlinedApi")
+        get() = if (getSDKVersion() >= Build.VERSION_CODES.Q) {
+            arrayOf(INTERNET, FOREGROUND_SERVICE)
+        } else if (getSDKVersion() >= P) {
+            arrayOf(INTERNET, WRITE_EXTERNAL_STORAGE, FOREGROUND_SERVICE)
+        } else {
+            arrayOf(INTERNET, WRITE_EXTERNAL_STORAGE)
+        }
+
+    @VisibleForTesting
+    internal fun getSDKVersion() = SDK_INT
 
     /**
      * Schedules a download through the [AbstractFetchDownloadService].

components/feature/downloads/src/test/java/mozilla/components/feature/downloads/manager/AndroidDownloadManagerTest.kt

@@ -9,6 +9,7 @@ import android.Manifest.permission.WRITE_EXTERNAL_STORAGE
 import android.app.DownloadManager.ACTION_DOWNLOAD_COMPLETE
 import android.app.DownloadManager.Request
 import android.content.Intent
+import android.os.Build
 import androidx.test.ext.junit.runners.AndroidJUnit4
 import mozilla.components.browser.state.state.content.DownloadState
 import mozilla.components.browser.state.store.BrowserStore
@@ -24,6 +25,8 @@ import org.junit.Before
 import org.junit.Test
 import org.junit.runner.RunWith
 import org.mockito.ArgumentMatchers.anyString
+import org.mockito.Mockito.doReturn
+import org.mockito.Mockito.spy
 import org.mockito.Mockito.verify
 import org.mockito.Mockito.verifyNoInteractions
 
@@ -94,6 +97,28 @@ class AndroidDownloadManagerTest {
         assertNull(id)
     }
 
+    @Test
+    fun `GIVEN a device that supports scoped storage THEN permissions must not included file access`() {
+        val downloadManager = spy(AndroidDownloadManager(testContext, store))
+
+        doReturn(Build.VERSION_CODES.Q).`when`(downloadManager).getSDKVersion()
+        println(downloadManager.permissions.joinToString { it })
+        assertTrue(WRITE_EXTERNAL_STORAGE !in downloadManager.permissions)
+    }
+
+    @Test
+    fun `GIVEN a device does not supports scoped storage THEN permissions must be included file access`() {
+        val downloadManager = spy(AndroidDownloadManager(testContext, store))
+
+        doReturn(Build.VERSION_CODES.P).`when`(downloadManager).getSDKVersion()
+
+        assertTrue(WRITE_EXTERNAL_STORAGE in downloadManager.permissions)
+
+        doReturn(Build.VERSION_CODES.O_MR1).`when`(downloadManager).getSDKVersion()
+
+        assertTrue(WRITE_EXTERNAL_STORAGE in downloadManager.permissions)
+    }
+
     @Test
     fun `sendBroadcast with valid downloadID must call onDownloadStopped after download`() {
         var downloadCompleted = false

components/feature/downloads/src/test/java/mozilla/components/feature/downloads/manager/FetchDownloadManagerTest.kt

@@ -11,6 +11,7 @@ import android.app.DownloadManager.ACTION_DOWNLOAD_COMPLETE
 import android.app.DownloadManager.EXTRA_DOWNLOAD_ID
 import android.content.Context
 import android.content.Intent
+import android.os.Build
 import androidx.localbroadcastmanager.content.LocalBroadcastManager
 import androidx.test.ext.junit.runners.AndroidJUnit4
 import mozilla.components.browser.state.state.content.DownloadState
@@ -30,7 +31,9 @@ import org.junit.Assert.assertTrue
 import org.junit.Before
 import org.junit.Test
 import org.junit.runner.RunWith
+import org.mockito.Mockito.doReturn
 import org.mockito.Mockito.never
+import org.mockito.Mockito.spy
 import org.mockito.Mockito.verify
 
 @RunWith(AndroidJUnit4::class)
@@ -101,6 +104,30 @@ class FetchDownloadManagerTest {
         assertTrue(downloadStopped)
     }
 
+    @Test
+    fun `GIVEN a device that supports scoped storage THEN permissions must not included file access`() {
+        downloadManager =
+            spy(FetchDownloadManager(mock(), store, MockDownloadService::class, broadcastManager))
+
+        doReturn(Build.VERSION_CODES.Q).`when`(downloadManager).getSDKVersion()
+
+        assertTrue(WRITE_EXTERNAL_STORAGE !in downloadManager.permissions)
+    }
+
+    @Test
+    fun `GIVEN a device does not supports scoped storage THEN permissions must be included file access`() {
+        downloadManager =
+            spy(FetchDownloadManager(mock(), store, MockDownloadService::class, broadcastManager))
+
+        doReturn(Build.VERSION_CODES.P).`when`(downloadManager).getSDKVersion()
+
+        assertTrue(WRITE_EXTERNAL_STORAGE in downloadManager.permissions)
+
+        doReturn(Build.VERSION_CODES.O_MR1).`when`(downloadManager).getSDKVersion()
+
+        assertTrue(WRITE_EXTERNAL_STORAGE in downloadManager.permissions)
+    }
+
     @Test
     fun `try again should not crash when download does not exist`() {
         val context: Context = mock()