Skip to content

Commit

Permalink
Closes mozilla-mobile#9441 ask for permissions system permissions onl…
Browse files Browse the repository at this point in the history 
…y when needed
  • Loading branch information
@hkaancaliskan @mergify
hkaancaliskan authored and mergify[bot] committed Jan 27, 2021
1 parent 6eba0ce commit 4b42536
Show file tree
Hide file tree
Showing 3 changed files with 131 additions and 46 deletions.
50 changes: 29 additions & 21 deletions .../feature/downloads/src/main/java/mozilla/components/feature/downloads/DownloadsFeature.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}

Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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())
Expand All @@ -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 = {
Expand Down
124 changes: 99 additions & 25 deletions ...ture/downloads/src/test/java/mozilla/components/feature/downloads/DownloadsFeatureTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<ConsumeDownloadUseCase>()
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<ConsumeDownloadUseCase>()
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
Expand Down Expand Up @@ -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,
Expand All @@ -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())
}

Expand Down Expand Up @@ -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<ConsumeDownloadUseCase>()
Expand All @@ -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)
Expand Down Expand Up @@ -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<DownloaderApp>()
val apps = listOf(ourApp, anotherApp)
val downloadManager: DownloadManager = mock()
var permissionsRequested = false
val dialog = DownloadAppChooserDialog()
val downloadsUseCases = spy(DownloadsUseCases(store))
val consumeDownloadUseCase = mock<ConsumeDownloadUseCase>()
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() {
Expand Down
3 changes: 3 additions & 0 deletions docs/changelog.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down

0 comments on commit 4b42536

Please sign in to comment.