From 4b4253694887f0be236facd6a03df3db3c2d2639 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hakk=C4=B1=20Kaan=20=C3=87al=C4=B1=C5=9Fkan?= Date: Tue, 19 Jan 2021 01:49:56 +0300 Subject: [PATCH] Closes #9441 ask for permissions system permissions only when needed --- .../feature/downloads/DownloadsFeature.kt | 50 ++++--- .../feature/downloads/DownloadsFeatureTest.kt | 124 ++++++++++++++---- docs/changelog.md | 3 + 3 files changed, 131 insertions(+), 46 deletions(-) diff --git a/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/DownloadsFeature.kt b/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/DownloadsFeature.kt index 5c63ad3d5fb..7ab5600d6cf 100644 --- a/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/DownloadsFeature.kt +++ b/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/DownloadsFeature.kt @@ -154,28 +154,26 @@ class DownloadsFeature( */ @VisibleForTesting internal fun processDownload(tab: SessionState, download: DownloadState): Boolean { - return if (applicationContext.isPermissionGranted(downloadManager.permissions.asIterable())) { + val apps = getDownloaderApps(applicationContext, download) + // We only show the dialog If we have multiple apps that can handle the download. + val shouldShowAppDownloaderDialog = shouldForwardToThirdParties() && apps.size > 1 - if (shouldForwardToThirdParties()) { - val apps = getDownloaderApps(applicationContext, download) - - // We only show the dialog If we have multiple apps that can handle the download. - if (apps.size > 1) { - showAppDownloaderDialog(tab, download, apps) - return false + return if (shouldShowAppDownloaderDialog) { + showAppDownloaderDialog(tab, download, apps) + false + } else { + if (applicationContext.isPermissionGranted(downloadManager.permissions.asIterable())) { + if (fragmentManager != null && !download.skipConfirmation) { + showDownloadDialog(tab, download) + false + } else { + useCases.consumeDownload(tab.id, download.id) + startDownload(download) } - } - - if (fragmentManager != null && !download.skipConfirmation) { - showDownloadDialog(tab, download) - false } else { - useCases.consumeDownload(tab.id, download.id) - startDownload(download) + onNeedToRequestPermissions(downloadManager.permissions) + false } - } else { - onNeedToRequestPermissions(downloadManager.permissions) - false } } @@ -204,7 +202,12 @@ class DownloadsFeature( withActiveDownload { (tab, download) -> if (applicationContext.isPermissionGranted(downloadManager.permissions.asIterable())) { - processDownload(tab, download) + if (shouldForwardToThirdParties()) { + startDownload(download) + useCases.consumeDownload(tab.id, download.id) + } else { + processDownload(tab, download) + } } else { useCases.consumeDownload(tab.id, download.id) } @@ -260,7 +263,12 @@ class DownloadsFeature( appChooserDialog.setApps(apps) appChooserDialog.onAppSelected = { app -> if (app.packageName == applicationContext.packageName) { - startDownload(download) + if (applicationContext.isPermissionGranted(downloadManager.permissions.asIterable())) { + startDownload(download) + useCases.consumeDownload(tab.id, download.id) + } else { + onNeedToRequestPermissions(downloadManager.permissions) + } } else { try { applicationContext.startActivity(app.toIntent()) @@ -271,8 +279,8 @@ class DownloadsFeature( ) Toast.makeText(applicationContext, errorMessage, Toast.LENGTH_SHORT).show() } + useCases.consumeDownload(tab.id, download.id) } - useCases.consumeDownload(tab.id, download.id) } appChooserDialog.onDismiss = { diff --git a/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/DownloadsFeatureTest.kt b/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/DownloadsFeatureTest.kt index e4ae38b00dc..9add8473d17 100644 --- a/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/DownloadsFeatureTest.kt +++ b/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/DownloadsFeatureTest.kt @@ -264,38 +264,69 @@ class DownloadsFeatureTest { } @Test - fun `onPermissionsResult will start download if permissions were granted`() { + fun `onPermissionsResult will start download if permissions were granted and thirdParty enabled`() { + val downloadsUseCases = spy(DownloadsUseCases(store)) + val consumeDownloadUseCase = mock() val download = DownloadState(url = "https://www.mozilla.org", sessionId = "test-tab") + val downloadManager: DownloadManager = mock() + val permissionsArray = arrayOf(INTERNET, WRITE_EXTERNAL_STORAGE) + val grantedPermissionsArray = arrayOf(PackageManager.PERMISSION_GRANTED, PackageManager.PERMISSION_GRANTED).toIntArray() + store.dispatch(ContentAction.UpdateDownloadAction("test-tab", download = download)) .joinBlocking() - val downloadManager: DownloadManager = mock() - doReturn( - arrayOf(INTERNET, WRITE_EXTERNAL_STORAGE) - ).`when`(downloadManager).permissions + doReturn(permissionsArray).`when`(downloadManager).permissions + doReturn(consumeDownloadUseCase).`when`(downloadsUseCases).consumeDownload - val feature = DownloadsFeature( + val feature = spy(DownloadsFeature( testContext, store, - useCases = DownloadsUseCases(store), - downloadManager = downloadManager - ) + useCases = downloadsUseCases, + downloadManager = downloadManager, + shouldForwardToThirdParties = { true } + )) - feature.start() + doReturn(false).`when`(feature).startDownload(any()) + + grantPermissions() + + feature.onPermissionsResult(permissionsArray, grantedPermissionsArray) - verify(downloadManager, never()).download(download) + verify(feature).startDownload(download) + verify(feature, never()).processDownload(any(), eq(download)) + verify(consumeDownloadUseCase).invoke(anyString(), anyString()) + } + + @Test + fun `onPermissionsResult will process download if permissions were granted and thirdParty disabled`() { + val downloadsUseCases = spy(DownloadsUseCases(store)) + val consumeDownloadUseCase = mock() + val download = DownloadState(url = "https://www.mozilla.org", sessionId = "test-tab") + val downloadManager: DownloadManager = mock() + val permissionsArray = arrayOf(INTERNET, WRITE_EXTERNAL_STORAGE) + val grantedPermissionsArray = arrayOf(PackageManager.PERMISSION_GRANTED, PackageManager.PERMISSION_GRANTED).toIntArray() + + store.dispatch(ContentAction.UpdateDownloadAction("test-tab", download = download)) + .joinBlocking() + + val feature = spy(DownloadsFeature( + testContext, + store, + useCases = downloadsUseCases, + downloadManager = downloadManager, + shouldForwardToThirdParties = { false } + )) + + doReturn(permissionsArray).`when`(downloadManager).permissions + doReturn(false).`when`(feature).processDownload(any(), any()) grantPermissions() - feature.onPermissionsResult( - arrayOf(INTERNET, WRITE_EXTERNAL_STORAGE), - arrayOf( - PackageManager.PERMISSION_GRANTED, - PackageManager.PERMISSION_GRANTED - ).toIntArray() - ) + feature.onPermissionsResult(permissionsArray, grantedPermissionsArray) - verify(downloadManager).download(download) + verify(feature).processDownload(any(), eq(download)) + verify(feature, never()).startDownload(download) + verify(consumeDownloadUseCase, never()).invoke(anyString(), anyString()) } @Test @@ -487,12 +518,10 @@ class DownloadsFeatureTest { fun `processDownload only forward downloads when shouldForwardToThirdParties is true`() { val tab = createTab("https://www.mozilla.org", id = "test-tab") val download = DownloadState(url = "https://www.mozilla.org/file.txt", sessionId = "test-tab") + val downloadManager: DownloadManager = mock() grantPermissions() - val downloadManager: DownloadManager = mock() - doReturn(arrayOf(INTERNET, WRITE_EXTERNAL_STORAGE)).`when`(downloadManager).permissions - val feature = spy(DownloadsFeature( testContext, store, @@ -501,11 +530,11 @@ class DownloadsFeatureTest { shouldForwardToThirdParties = { false } )) + doReturn(arrayOf(INTERNET, WRITE_EXTERNAL_STORAGE)).`when`(downloadManager).permissions doReturn(false).`when`(feature).startDownload(download) feature.processDownload(tab, download) - verify(feature, never()).getDownloaderApps(testContext, download) verify(feature, never()).showAppDownloaderDialog(any(), any(), any(), any()) } @@ -621,7 +650,7 @@ class DownloadsFeatureTest { } @Test - fun `when our app is selected for downloading we should perform the download`() { + fun `when our app is selected for downloading and permission granted then we should perform the download`() { val spyContext = spy(testContext) val downloadsUseCases = spy(DownloadsUseCases(store)) val consumeDownloadUseCase = mock() @@ -632,18 +661,22 @@ class DownloadsFeatureTest { val apps = listOf(ourApp, anotherApp) val dialog = DownloadAppChooserDialog() val fragmentManager: FragmentManager = mockFragmentManager() + val downloadManager: DownloadManager = mock() val feature = spy(DownloadsFeature( testContext, store, downloadsUseCases, - downloadManager = mock(), + downloadManager = downloadManager, shouldForwardToThirdParties = { true }, fragmentManager = fragmentManager )) + grantPermissions() + doReturn(dialog).`when`(fragmentManager).findFragmentByTag(DownloadAppChooserDialog.FRAGMENT_TAG) doReturn(consumeDownloadUseCase).`when`(downloadsUseCases).consumeDownload doReturn(false).`when`(feature).startDownload(any()) + doReturn(arrayOf(INTERNET, WRITE_EXTERNAL_STORAGE)).`when`(downloadManager).permissions feature.showAppDownloaderDialog(tab, download, apps) dialog.onAppSelected(ourApp) @@ -841,6 +874,47 @@ class DownloadsFeatureTest { verify(downloadsUseCases, never()).consumeDownload assertNotNull(feature.previousTab) } + + @Test + fun `when our app is selected for downloading and permission not granted then we should ask for permission`() { + val tab = createTab("https://www.mozilla.org", id = "test-tab") + val download = DownloadState(url = "https://www.mozilla.org/file.txt", sessionId = "test-tab") + val ourApp = DownloaderApp(name = "app", packageName = testContext.packageName, resolver = mock(), activityName = "", url = "", contentType = null) + val anotherApp = mock() + val apps = listOf(ourApp, anotherApp) + val downloadManager: DownloadManager = mock() + var permissionsRequested = false + val dialog = DownloadAppChooserDialog() + val downloadsUseCases = spy(DownloadsUseCases(store)) + val consumeDownloadUseCase = mock() + val fragmentManager: FragmentManager = mockFragmentManager() + + val feature = spy(DownloadsFeature( + testContext, + store, + useCases = downloadsUseCases, + downloadManager = downloadManager, + shouldForwardToThirdParties = { true }, + onNeedToRequestPermissions = { permissionsRequested = true }, + fragmentManager = fragmentManager + )) + + doReturn(arrayOf(INTERNET, WRITE_EXTERNAL_STORAGE)).`when`(downloadManager).permissions + doReturn(testContext.packageName).`when`(spy(ourApp)).packageName + doReturn(dialog).`when`(fragmentManager).findFragmentByTag(DownloadAppChooserDialog.FRAGMENT_TAG) + doReturn(consumeDownloadUseCase).`when`(downloadsUseCases).consumeDownload + + assertFalse(permissionsRequested) + + feature.showAppDownloaderDialog(tab, download, apps, dialog) + dialog.onAppSelected(ourApp) + + assertTrue(permissionsRequested) + + verify(feature, never()).startDownload(any()) + verify(spy(testContext), never()).startActivity(any()) + verify(consumeDownloadUseCase, never()).invoke(anyString(), anyString()) + } } private fun grantPermissions() { diff --git a/docs/changelog.md b/docs/changelog.md index 423ee42a2d3..96f26ee9842 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -26,6 +26,9 @@ permalink: /changelog/ * **support-base** * ⚠️ **This is a breaking change**: Update the signature of `ActivityResultHandler.onActivityResult` to avoid conflict with internal Android APIs. +* **feature-downloads**: + * 🚒 Bug fixed [issue #9441](https://github.com/mozilla-mobile/android-components/issues/9441) - Don't ask for redundant system files permission if not required. + * **feature-addons** * 🚒 Bug fixed [issue #9484] https://github.com/mozilla-mobile/android-components/issues/9484) - Handle multiple add-ons update that require new permissions.