From ed067fc447465c86331c431e37cd1712cff771ba Mon Sep 17 00:00:00 2001 From: czlucius <58442255+czlucius@users.noreply.github.com> Date: Fri, 23 Jul 2021 14:50:31 +0800 Subject: [PATCH] Close #9830 Do no require write storage permission to downloading files on devices with Android 10 and above --- .../downloads/src/main/AndroidManifest.xml | 3 ++- .../manager/AndroidDownloadManager.kt | 21 +++++++++++---- .../downloads/manager/FetchDownloadManager.kt | 21 +++++++++++---- .../manager/AndroidDownloadManagerTest.kt | 25 +++++++++++++++++ .../manager/FetchDownloadManagerTest.kt | 27 +++++++++++++++++++ 5 files changed, 86 insertions(+), 11 deletions(-) diff --git a/components/feature/downloads/src/main/AndroidManifest.xml b/components/feature/downloads/src/main/AndroidManifest.xml index 6b97a2a7722..a9c5bc2da04 100644 --- a/components/feature/downloads/src/main/AndroidManifest.xml +++ b/components/feature/downloads/src/main/AndroidManifest.xml @@ -8,7 +8,8 @@ package="mozilla.components.feature.downloads"> + tools:ignore="ScopedStorage" + android:maxSdkVersion="29"/> diff --git a/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/manager/AndroidDownloadManager.kt b/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/manager/AndroidDownloadManager.kt index dd015be5efd..ddc2df885f9 100644 --- a/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/manager/AndroidDownloadManager.kt +++ b/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,7 +48,16 @@ class AndroidDownloadManager( private val downloadRequests = LongSparseArray() 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]. @@ -54,7 +65,6 @@ class AndroidDownloadManager( * @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) diff --git a/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/manager/FetchDownloadManager.kt b/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/manager/FetchDownloadManager.kt index 4743654a2df..0702b2ba8f2 100644 --- a/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/manager/FetchDownloadManager.kt +++ b/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( 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]. diff --git a/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/manager/AndroidDownloadManagerTest.kt b/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/manager/AndroidDownloadManagerTest.kt index 85d39872c83..8458ff131f2 100644 --- a/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/manager/AndroidDownloadManagerTest.kt +++ b/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 diff --git a/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/manager/FetchDownloadManagerTest.kt b/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/manager/FetchDownloadManagerTest.kt index 74e9a032845..00660920709 100644 --- a/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/manager/FetchDownloadManagerTest.kt +++ b/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()