Skip to content

Commit

Permalink
Closes mozilla-mobile#6565: Handle deny & don't ask again in SitePerm…
Browse files Browse the repository at this point in the history 
…issionsFeature
  • Loading branch information
@Amejia481
Amejia481 committed Apr 7, 2020
1 parent 7b10896 commit 2d2cfca
Show file tree
Hide file tree
Showing 5 changed files with 107 additions and 41 deletions.
63 changes: 40 additions & 23 deletions ...ssions/src/main/java/mozilla/components/feature/sitepermissions/SitePermissionsFeature.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ import mozilla.components.concept.engine.permission.Permission.ContentGeoLocatio
import mozilla.components.concept.engine.permission.Permission.ContentNotification
import mozilla.components.concept.engine.permission.Permission.ContentVideoCamera
import mozilla.components.concept.engine.permission.Permission.ContentVideoCapture
import mozilla.components.concept.engine.permission.Permission.AppLocationCoarse
import mozilla.components.concept.engine.permission.Permission.AppLocationFine
import mozilla.components.concept.engine.permission.Permission.AppAudio
import mozilla.components.concept.engine.permission.Permission.AppCamera
import mozilla.components.concept.engine.permission.PermissionRequest
import mozilla.components.feature.sitepermissions.SitePermissions.Status.ALLOWED
import mozilla.components.feature.sitepermissions.SitePermissions.Status.BLOCKED
Expand All @@ -41,6 +45,7 @@ import mozilla.components.support.base.feature.LifecycleAwareFeature
import mozilla.components.support.base.feature.OnNeedToRequestPermissions
import mozilla.components.support.base.feature.PermissionsFeature
import mozilla.components.support.ktx.android.content.isPermissionGranted
import java.lang.IllegalStateException
import java.security.InvalidParameterException

internal const val FRAGMENT_TAG = "mozac_feature_sitepermissions_prompt_dialog"
Expand All @@ -62,6 +67,8 @@ internal const val FRAGMENT_TAG = "mozac_feature_sitepermissions_prompt_dialog"
* @property dialogConfig optional customization for dialog initial state. See [DialogConfig].
* @property onNeedToRequestPermissions a callback invoked when permissions
* need to be requested. Once the request is completed, [onPermissionsResult] needs to be invoked.
* @property onShouldShowRequestPermissionRationale a callback that allows the feature to query
* the ActivityCompat.shouldShowRequestPermissionRationale or the Fragment.shouldShowRequestPermissionRationale values.
**/

@Suppress("TooManyFunctions", "LargeClass")
Expand All @@ -74,7 +81,8 @@ class SitePermissionsFeature(
private val fragmentManager: FragmentManager,
var promptsStyling: PromptsStyling? = null,
private val dialogConfig: DialogConfig? = null,
override val onNeedToRequestPermissions: OnNeedToRequestPermissions
override val onNeedToRequestPermissions: OnNeedToRequestPermissions,
val onShouldShowRequestPermissionRationale: (permission: String) -> Boolean
) : LifecycleAwareFeature, PermissionsFeature {

private val observer = SitePermissionsRequestObserver(sessionManager, feature = this)
Expand Down Expand Up @@ -105,6 +113,7 @@ class SitePermissionsFeature(
* @param grantResults the grant results for the corresponding permissions
* @see [onNeedToRequestPermissions].
*/
@Suppress("MaxLineLength")
override fun onPermissionsResult(permissions: Array<String>, grantResults: IntArray) {
sessionManager.runWithSessionIdOrSelected(sessionId) { session ->
session.appPermissionRequest.consume { permissionsRequest ->
Expand All @@ -117,6 +126,14 @@ class SitePermissionsFeature(
permissionsRequest.grant()
} else {
permissionsRequest.reject()
permissions.forEach { systemPermission ->
if (!onShouldShowRequestPermissionRationale(systemPermission)) {
// The system permission is denied permanently
val appPermission = listOf(permissionsRequest.permissions.find { it.id == systemPermission }
?: throw IllegalStateException("$systemPermission is not part of the permission request"))
storeSitePermissions(session, permissionsRequest, appPermission, status = BLOCKED)
}
}
}
true
}
Expand Down Expand Up @@ -173,7 +190,6 @@ class SitePermissionsFeature(
}
}

@Synchronized
internal fun storeSitePermissions(
session: Session,
request: PermissionRequest,
Expand All @@ -183,19 +199,17 @@ class SitePermissionsFeature(
if (session.private) {
return
}

ioCoroutineScope.launch {
var sitePermissions = storage.findSitePermissionsBy(request.host)
synchronized(storage) {
var sitePermissions = storage.findSitePermissionsBy(request.getHost(session))

if (sitePermissions == null) {
sitePermissions = request.toSitePermissions(
status = status,
permissions = permissions
)
storage.save(sitePermissions)
} else {
sitePermissions = request.toSitePermissions(status, sitePermissions)
storage.update(sitePermissions)
if (sitePermissions == null) {
sitePermissions = request.toSitePermissions(session, status = status, permissions = permissions)
storage.save(sitePermissions)
} else {
sitePermissions = request.toSitePermissions(session, status, sitePermissions)
storage.update(sitePermissions)
}
}
}
}
Expand Down Expand Up @@ -231,7 +245,7 @@ class SitePermissionsFeature(
}

val permissionFromStorage = withContext(ioCoroutineScope.coroutineContext) {
storage.findSitePermissionsBy(request.host)
storage.findSitePermissionsBy(request.getHost(session))
}

val prompt = if (shouldApplyRules(permissionFromStorage) ||
Expand Down Expand Up @@ -323,8 +337,9 @@ class SitePermissionsFeature(
}

private fun PermissionRequest.toSitePermissions(
session: Session,
status: SitePermissions.Status,
initialSitePermission: SitePermissions = getInitialSitePermissions(this),
initialSitePermission: SitePermissions = getInitialSitePermissions(session, this),
permissions: List<Permission> = this.permissions
): SitePermissions {
var sitePermissions = initialSitePermission
Expand All @@ -334,10 +349,10 @@ class SitePermissionsFeature(
return sitePermissions
}

internal fun getInitialSitePermissions(request: PermissionRequest): SitePermissions {
internal fun getInitialSitePermissions(session: Session, request: PermissionRequest): SitePermissions {
val rules = sitePermissionsRules
return rules?.toSitePermissions(request.host, savedAt = System.currentTimeMillis())
?: SitePermissions(request.host, savedAt = System.currentTimeMillis())
return rules?.toSitePermissions(request.getHost(session), savedAt = System.currentTimeMillis())
?: SitePermissions(request.getHost(session), savedAt = System.currentTimeMillis())
}

private fun PermissionRequest.isForAutoplay() =
Expand All @@ -349,16 +364,16 @@ class SitePermissionsFeature(
sitePermissions: SitePermissions
): SitePermissions {
return when (permission) {
is ContentGeoLocation -> {
is ContentGeoLocation, is AppLocationCoarse, is AppLocationFine -> {
sitePermissions.copy(location = status)
}
is ContentNotification -> {
sitePermissions.copy(notification = status)
}
is ContentAudioCapture, is ContentAudioMicrophone -> {
is ContentAudioCapture, is ContentAudioMicrophone, is AppAudio -> {
sitePermissions.copy(microphone = status)
}
is ContentVideoCamera, is ContentVideoCapture -> {
is ContentVideoCamera, is ContentVideoCapture, is AppCamera -> {
sitePermissions.copy(camera = status)
}
is ContentAutoPlayAudible -> {
Expand All @@ -376,7 +391,7 @@ class SitePermissionsFeature(
permissionRequest: PermissionRequest,
session: Session
): SitePermissionsDialogFragment {
val host = permissionRequest.host
val host = permissionRequest.getHost(session)
return if (!permissionRequest.containsVideoAndAudioSources()) {
val permission = permissionRequest.permissions.first()
handlingSingleContentPermissions(session.id, permission, host)
Expand Down Expand Up @@ -484,7 +499,9 @@ class SitePermissionsFeature(
return false
}

private val PermissionRequest.host get() = uri?.toUri()?.host ?: ""
private fun PermissionRequest.getHost(session: Session) = (uri?.toUri()?.host
?: "").ifEmpty { session.url.toUri().host ?: "" }

private val PermissionRequest.isMedia: Boolean
get() {
return when (permissions.first()) {
Expand Down
69 changes: 56 additions & 13 deletions ...ns/src/test/java/mozilla/components/feature/sitepermissions/SitePermissionsFeatureTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,8 @@ class SitePermissionsFeatureTest {
sessionManager = mockSessionManager,
onNeedToRequestPermissions = mockOnNeedToRequestPermissions,
storage = mockStorage,
fragmentManager = mockFragmentManager
fragmentManager = mockFragmentManager,
onShouldShowRequestPermissionRationale = mock()
)
}

Expand All @@ -90,7 +91,8 @@ class SitePermissionsFeatureTest {
onNeedToRequestPermissions = {
wasCalled = true
},
fragmentManager = mockFragmentManager
fragmentManager = mockFragmentManager,
onShouldShowRequestPermissionRationale = mock()
)

sitePermissionFeature.start()
Expand All @@ -101,6 +103,35 @@ class SitePermissionsFeatureTest {
assertTrue(wasCalled)
}

@Test
fun `when users click the deny & don't ask again button in a system permission dialog, we must store the site permission`() {
val session = getSelectedSession()
var wasCalled = false
val mockPermissionRequest: PermissionRequest = mock {
whenever(permissions).thenReturn(listOf(Permission.AppAudio(id = "permission")))
}
val feature = SitePermissionsFeature(
context = testContext,
sessionManager = mockSessionManager,
onShouldShowRequestPermissionRationale = {
wasCalled = true
false
},
fragmentManager = mockFragmentManager,
onNeedToRequestPermissions = mock(),
storage = mockStorage
)
runBlocking {
feature.coroutineScopeInitializer = { this }
session.appPermissionRequest = Consumable.from(mockPermissionRequest)

feature.onPermissionsResult(arrayOf("permission"), arrayOf<Int>().toIntArray())
assertTrue(wasCalled)
}
verify(mockStorage).findSitePermissionsBy(anyString())
verify(mockStorage).save(any())
}

@Test
fun `requesting an invalid content permission request will reject the permission request`() {
val session = getSelectedSession()
Expand All @@ -110,7 +141,8 @@ class SitePermissionsFeatureTest {
sessionManager = mockSessionManager,
onNeedToRequestPermissions = {
},
fragmentManager = mockFragmentManager
fragmentManager = mockFragmentManager,
onShouldShowRequestPermissionRationale = mock()
)

sitePermissionFeature.start()
Expand All @@ -137,7 +169,8 @@ class SitePermissionsFeatureTest {
onNeedToRequestPermissions = {
wasCalled = true
},
fragmentManager = mockFragmentManager
fragmentManager = mockFragmentManager,
onShouldShowRequestPermissionRationale = mock()
)

sitePermissionFeature.start()
Expand Down Expand Up @@ -173,7 +206,8 @@ class SitePermissionsFeatureTest {
wasCalled = true
},
sessionId = customTabSession.id,
fragmentManager = mockFragmentManager
fragmentManager = mockFragmentManager,
onShouldShowRequestPermissionRationale = mock()
)

sitePermissionFeature.start()
Expand Down Expand Up @@ -418,12 +452,14 @@ class SitePermissionsFeatureTest {
fun `storing a new SitePermissions must call save on the store`() {
val sitePermissionsList = listOf(ContentGeoLocation())
val request: PermissionRequest = mock()

val mockSession: Session = mock {
whenever(url).thenReturn(" http://mozilla.org")
}
runBlocking {
sitePermissionFeature.coroutineScopeInitializer = {
this
}
sitePermissionFeature.storeSitePermissions(mock(), request, sitePermissionsList, ALLOWED)
sitePermissionFeature.storeSitePermissions(mockSession, request, sitePermissionsList, ALLOWED)
}
verify(mockStorage).findSitePermissionsBy(anyString())
verify(mockStorage).save(any())
Expand Down Expand Up @@ -465,6 +501,9 @@ class SitePermissionsFeatureTest {
val mockRequest: PermissionRequest = mock()
val sitePermissionFromStorage: SitePermissions = mock()
val permissions = listOf(permission)
val mockSession: Session = mock {
whenever(url).thenReturn(" http://mozilla.org")
}
doReturn(sitePermissionFromStorage).`when`(mockStorage).findSitePermissionsBy(anyString())
doReturn(permissions).`when`(mockRequest).permissions

Expand All @@ -473,13 +512,14 @@ class SitePermissionsFeatureTest {
sessionManager = mockSessionManager,
fragmentManager = mockFragmentManager,
onNeedToRequestPermissions = mockOnNeedToRequestPermissions,
storage = mockStorage
storage = mockStorage,
onShouldShowRequestPermissionRationale = mock()
)

try {
runBlocking {
feature.coroutineScopeInitializer = { this }
feature.storeSitePermissions(mock(), mockRequest, permissions, ALLOWED)
feature.storeSitePermissions(mockSession, mockRequest, permissions, ALLOWED)
}
verify(mockStorage, times(index + 1)).findSitePermissionsBy(anyString())
verify(mockStorage, times(index + 1)).update(any())
Expand All @@ -493,7 +533,9 @@ class SitePermissionsFeatureTest {
@Test
fun `requesting a content permissions with an already stored allowed permissions will auto granted it and not show a prompt`() {
val request: PermissionRequest = mock()
val mockSession: Session = mock()
val mockSession: Session = mock {
whenever(url).thenReturn(" http://mozilla.org")
}
val mockConsumable: Consumable<PermissionRequest> = mock()
val sitePermissionFromStorage: SitePermissions = mock()
val permissionList = listOf(ContentGeoLocation())
Expand All @@ -515,7 +557,7 @@ class SitePermissionsFeatureTest {
@Test
fun `requesting a content permissions with an already stored blocked permission will auto block it and not show a prompt`() {
val request: PermissionRequest = mock()
val mockSession: Session = mock()
val mockSession: Session = mock { whenever(url).thenReturn(" http://mozilla.org") }
val mockConsumable: Consumable<PermissionRequest> = mock()
val sitePermissionFromStorage: SitePermissions = mock()
val permissionList = listOf(ContentGeoLocation())
Expand Down Expand Up @@ -837,7 +879,8 @@ class SitePermissionsFeatureTest {
sessionManager = mockSessionManager,
onNeedToRequestPermissions = mockOnNeedToRequestPermissions,
storage = mockStorage,
fragmentManager = mockFragmentManager
fragmentManager = mockFragmentManager,
onShouldShowRequestPermissionRationale = mock()
)

mockSessionManager.add(session)
Expand Down Expand Up @@ -938,7 +981,7 @@ class SitePermissionsFeatureTest {
whenever(uri).thenReturn(" http://mozilla.org")
}

val sitePermissions = sitePermissionFeature.getInitialSitePermissions(mockPermissionRequest)
val sitePermissions = sitePermissionFeature.getInitialSitePermissions(mock(), mockPermissionRequest)

assertEquals("mozilla.org", sitePermissions.origin)
assertEquals(BLOCKED, sitePermissions.location)
Expand Down
3 changes: 2 additions & 1 deletion ...ions/src/test/java/mozilla/components/feature/sitepermissions/SitePermissionsRulesTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ class SitePermissionsRulesTest {
sessionManager = mockSessionManager,
onNeedToRequestPermissions = mockOnNeedToRequestPermissions,
storage = mockStorage,
fragmentManager = mock()
fragmentManager = mock(),
onShouldShowRequestPermissionRationale = mock()
)
}

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 @@ -76,6 +76,9 @@ permalink: /changelog/
* Added an optional URL view to the `TabViewHolder` to display the URL.
* Will expose a new `layout` parameter which allows consumers to change the tabs tray layout.

* **feature-sitepermissions**
* ⚠️ **This is a breaking change**: The `SitePermissionsFeature`'s constructor, now requires a new parameter `onShouldShowRequestPermissionRationale` a lambda to allow the feature to query [ActivityCompat.shouldShowRequestPermissionRationale](https://developer.android.com/reference/androidx/core/app/ActivityCompat#shouldShowRequestPermissionRationale(android.app.Activity,%20java.lang.String)) or [Fragment.shouldShowRequestPermissionRationale](https://developer.android.com/reference/androidx/fragment/app/Fragment#shouldShowRequestPermissionRationale(java.lang.String)). This allows the `SitePermissionsFeature` to handle when a user clicks "Deny & don't ask again" button in a system permission dialog, for more information see [issue #6565](https://github.com/mozilla-mobile/android-components/issues/6565).

# 37.0.0

* [Commits](https://github.com/mozilla-mobile/android-components/compare/v36.0.0...v37.0.0)
Expand Down
10 changes: 6 additions & 4 deletions samples/browser/src/main/java/org/mozilla/samples/browser/BaseBrowserFragment.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -175,10 +175,12 @@ abstract class BaseBrowserFragment : Fragment(), UserInteractionHandler {
location = SitePermissionsRules.Action.ASK_TO_ALLOW,
notification = SitePermissionsRules.Action.ASK_TO_ALLOW,
microphone = SitePermissionsRules.Action.ASK_TO_ALLOW
)
) { permissions ->
requestPermissions(permissions, REQUEST_CODE_APP_PERMISSIONS)
},
),
onNeedToRequestPermissions = { permissions ->
requestPermissions(permissions, REQUEST_CODE_APP_PERMISSIONS)
},
onShouldShowRequestPermissionRationale = { shouldShowRequestPermissionRationale(it) }
),
owner = this,
view = layout
)
Expand Down

0 comments on commit 2d2cfca

Please sign in to comment.