Close mozilla-mobile#9830 Do no require write storage permission to d…

…ownloading files on devices with Android 10 and above

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

@@ -15,7 +15,7 @@ import android.content.Context
 import android.content.Intent
 import android.content.IntentFilter
 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
@@ -29,6 +29,7 @@ import mozilla.components.concept.fetch.Headers.Names.USER_AGENT
 import mozilla.components.feature.downloads.AbstractFetchDownloadService
 import mozilla.components.feature.downloads.ext.isScheme
 import mozilla.components.support.utils.DownloadUtils
+import android.os.Build
 
 typealias SystemDownloadManager = android.app.DownloadManager
 typealias SystemRequest = android.app.DownloadManager.Request
@@ -47,18 +48,23 @@ class AndroidDownloadManager(
     private val downloadRequests = LongSparseArray<SystemRequest>()
     private var isSubscribedReceiver = false
 
-    // Do not require WRITE_EXTERNAL_STORAGE permission on API 30 and above (using scoped storage)
-    override val permissions =
-        if (SDK_INT >= Build.VERSION_CODES.R) arrayOf(INTERNET)
-        else 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 = permissions.toList())
     override fun download(download: DownloadState, cookie: String): String? {
         val androidDownloadManager: SystemDownloadManager = applicationContext.getSystemService()!!
 
@@ -110,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

@@ -25,7 +25,10 @@ import org.junit.Test
 import org.junit.runner.RunWith
 import org.mockito.ArgumentMatchers.anyString
 import org.mockito.Mockito.verify
+import org.mockito.Mockito.spy
 import org.mockito.Mockito.verifyZeroInteractions
+import org.mockito.Mockito.doReturn
+import android.os.Build
 
 @RunWith(AndroidJUnit4::class)
 class AndroidDownloadManagerTest {
@@ -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
@@ -32,6 +33,8 @@ import org.junit.Test
 import org.junit.runner.RunWith
 import org.mockito.Mockito.never
 import org.mockito.Mockito.verify
+import org.mockito.Mockito.spy
+import org.mockito.Mockito.doReturn
 
 @RunWith(AndroidJUnit4::class)
 class FetchDownloadManagerTest {
@@ -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()