diff --git a/components/browser/state/src/main/java/mozilla/components/browser/state/action/BrowserAction.kt b/components/browser/state/src/main/java/mozilla/components/browser/state/action/BrowserAction.kt index a6ce581df2c..6d4a0b61115 100644 --- a/components/browser/state/src/main/java/mozilla/components/browser/state/action/BrowserAction.kt +++ b/components/browser/state/src/main/java/mozilla/components/browser/state/action/BrowserAction.kt @@ -398,8 +398,7 @@ sealed class ContentAction : BrowserAction() { ) : ContentAction() /** - * Adds a new unprocessed content permission request to the [ContentState] list, to be - * processed in [mozilla.components.feature.sitepermissions.SitePermissionsFeature] + * Adds a new content permission request to the [ContentState] list. * */ data class UpdatePermissionsRequest( val sessionId: String, @@ -407,8 +406,7 @@ sealed class ContentAction : BrowserAction() { ) : ContentAction() /** - * Deletes a content permission request from the [ContentState] list, has been - * processed in [mozilla.components.feature.sitepermissions.SitePermissionsFeature] + * Deletes a content permission request from the [ContentState] list. * */ data class ConsumePermissionsRequest( val sessionId: String, @@ -416,8 +414,7 @@ sealed class ContentAction : BrowserAction() { ) : ContentAction() /** - * Adds a new unprocessed app permission request to the [ContentState] list, to be - * processed in [mozilla.components.feature.sitepermissions.SitePermissionsFeature] + * Adds a new app permission request to the [ContentState] list. * */ data class UpdateAppPermissionsRequest( val sessionId: String, @@ -425,8 +422,7 @@ sealed class ContentAction : BrowserAction() { ) : ContentAction() /** - * Deletes an app permission request from the [ContentState] list, has been - * processed in [mozilla.components.feature.sitepermissions.SitePermissionsFeature] + * Deletes an app permission request from the [ContentState] list. * */ data class ConsumeAppPermissionsRequest( val sessionId: String, diff --git a/components/browser/state/src/main/java/mozilla/components/browser/state/ext/PermissionRequest.kt b/components/browser/state/src/main/java/mozilla/components/browser/state/ext/PermissionRequest.kt index 931e3091152..19db869e382 100644 --- a/components/browser/state/src/main/java/mozilla/components/browser/state/ext/PermissionRequest.kt +++ b/components/browser/state/src/main/java/mozilla/components/browser/state/ext/PermissionRequest.kt @@ -5,8 +5,7 @@ package mozilla.components.browser.state.ext import mozilla.components.concept.engine.permission.PermissionRequest /** - * An improved version of [List.contains] built to be used in [ContentStateReducer] to make sure - * no duplicate requests are added to the state. + * @returns true if the given [permissionRequest] is contained in this list otherwise false * */ fun List.doesContain(permissionRequest: PermissionRequest): Boolean { return this.any { diff --git a/components/browser/state/src/main/java/mozilla/components/browser/state/reducer/ContentStateReducer.kt b/components/browser/state/src/main/java/mozilla/components/browser/state/reducer/ContentStateReducer.kt index e7633366c66..8a5d17cdf8d 100644 --- a/components/browser/state/src/main/java/mozilla/components/browser/state/reducer/ContentStateReducer.kt +++ b/components/browser/state/src/main/java/mozilla/components/browser/state/reducer/ContentStateReducer.kt @@ -11,6 +11,7 @@ import mozilla.components.browser.state.state.BrowserState import mozilla.components.browser.state.state.ContentState import mozilla.components.browser.state.state.SessionState import mozilla.components.browser.state.state.content.HistoryState +import mozilla.components.concept.engine.permission.PermissionRequest import mozilla.components.support.ktx.android.net.sameSchemeAndHostAs internal object ContentStateReducer { @@ -134,10 +135,8 @@ internal object ContentStateReducer { action.sessionId ) { if (!it.permissionRequestsList.doesContain(action.permissionRequest)) { - val currentList = it.permissionRequestsList.toMutableList() - currentList.add(action.permissionRequest) it.copy( - permissionRequestsList = currentList + permissionRequestsList = it.permissionRequestsList + action.permissionRequest ) } else { it @@ -148,10 +147,8 @@ internal object ContentStateReducer { action.sessionId ) { if (it.permissionRequestsList.doesContain(action.permissionRequest)) { - val newList = it.permissionRequestsList.toMutableList() - newList.remove(action.permissionRequest) it.copy( - permissionRequestsList = newList + permissionRequestsList = it.permissionRequestsList - action.permissionRequest ) } else { it @@ -162,10 +159,8 @@ internal object ContentStateReducer { action.sessionId ) { if (!it.appPermissionRequestsList.doesContain(action.appPermissionRequest)) { - val currentList = it.appPermissionRequestsList.toMutableList() - currentList.add(action.appPermissionRequest) it.copy( - appPermissionRequestsList = currentList + appPermissionRequestsList = it.appPermissionRequestsList + action.appPermissionRequest ) } else { it @@ -176,10 +171,8 @@ internal object ContentStateReducer { action.sessionId ) { if (it.appPermissionRequestsList.doesContain(action.appPermissionRequest)) { - val newList = it.appPermissionRequestsList.toMutableList() - newList.remove(action.appPermissionRequest) it.copy( - appPermissionRequestsList = newList + appPermissionRequestsList = it.appPermissionRequestsList - action.appPermissionRequest ) } else { it diff --git a/components/browser/state/src/main/java/mozilla/components/browser/state/state/ContentState.kt b/components/browser/state/src/main/java/mozilla/components/browser/state/state/ContentState.kt index 97a47c5c7a2..664ad1feeff 100644 --- a/components/browser/state/src/main/java/mozilla/components/browser/state/state/ContentState.kt +++ b/components/browser/state/src/main/java/mozilla/components/browser/state/state/ContentState.kt @@ -43,8 +43,8 @@ import mozilla.components.concept.engine.window.WindowRequest * @property webAppManifest the Web App Manifest for the currently visited page (or null). * @property firstContentfulPaint whether or not the first contentful paint has happened. * @property pictureInPictureEnabled True if the session is being displayed in PIP mode. - * @property permissionRequestsList Holds unprocessed content requests sent from EngineObserver. - * @property appPermissionRequestsList Holds unprocessed app requests sent from EngineObserver. + * @property permissionRequestsList Holds unprocessed content requests. + * @property appPermissionRequestsList Holds unprocessed app requests. */ data class ContentState( val url: String, diff --git a/components/feature/sitepermissions/src/main/java/mozilla/components/feature/sitepermissions/SitePermissionsFeature.kt b/components/feature/sitepermissions/src/main/java/mozilla/components/feature/sitepermissions/SitePermissionsFeature.kt index 38d2c77b1d4..3d209ec268a 100644 --- a/components/feature/sitepermissions/src/main/java/mozilla/components/feature/sitepermissions/SitePermissionsFeature.kt +++ b/components/feature/sitepermissions/src/main/java/mozilla/components/feature/sitepermissions/SitePermissionsFeature.kt @@ -12,6 +12,7 @@ import android.content.pm.PackageManager import androidx.annotation.ColorRes import androidx.annotation.DrawableRes import androidx.annotation.StringRes +import androidx.annotation.VisibleForTesting import androidx.core.net.toUri import androidx.fragment.app.FragmentManager import kotlinx.coroutines.CoroutineScope @@ -24,10 +25,9 @@ import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.cancel import mozilla.components.browser.session.Session import mozilla.components.browser.session.SessionManager -import mozilla.components.browser.session.runWithSession -import mozilla.components.browser.session.runWithSessionIdOrSelected import mozilla.components.browser.state.action.ContentAction import mozilla.components.browser.state.selector.findTabOrCustomTabOrSelectedTab +import mozilla.components.browser.state.state.ContentState import mozilla.components.browser.state.store.BrowserStore import mozilla.components.concept.engine.permission.Permission import mozilla.components.concept.engine.permission.Permission.ContentAudioCapture @@ -121,7 +121,7 @@ class SitePermissionsFeature( .collect { permissionRequest -> val host = - store.state.findTabOrCustomTabOrSelectedTab(customTabId)?.content?.url + getCurrentContentState()?.url ?: "" if (permissionRequest.permissions.all { it.isSupported() }) { @@ -149,8 +149,12 @@ class SitePermissionsFeature( } } - private fun consumePermissionRequest(permissionRequest: PermissionRequest) { - sessionId?.let { sessionId -> + private fun consumePermissionRequest( + permissionRequest: PermissionRequest, + optionalSessionId: String? = null + ) { + val thisSessionId = optionalSessionId ?: sessionId + thisSessionId?.let { sessionId -> store.dispatch(ContentAction.ConsumePermissionsRequest(sessionId, permissionRequest)) } } @@ -179,32 +183,40 @@ class SitePermissionsFeature( */ @Suppress("MaxLineLength") override fun onPermissionsResult(permissions: Array, grantResults: IntArray) { - sessionManager.runWithSessionIdOrSelected(sessionId) { session -> + val currentContentSate = getCurrentContentState() + val appPermissionRequest = findRequestedAppPermission(permissions) - val appPermissionRequest = - store.state.findTabOrCustomTabOrSelectedTab(customTabId)?.content?.appPermissionRequestsList?.find { - permissions.contains(it.permissions.first().id) - } - - if (appPermissionRequest != null) { - val allPermissionWereGranted = grantResults.all { grantResult -> - grantResult == PackageManager.PERMISSION_GRANTED - } + if (appPermissionRequest != null && currentContentSate != null) { + val allPermissionWereGranted = grantResults.all { grantResult -> + grantResult == PackageManager.PERMISSION_GRANTED + } - if (grantResults.isNotEmpty() && allPermissionWereGranted) { - appPermissionRequest.grant() - } else { - appPermissionRequest.reject() - permissions.forEach { systemPermission -> - if (!onShouldShowRequestPermissionRationale(systemPermission)) { - // The system permission is denied permanently - storeSitePermissions(session, appPermissionRequest, status = BLOCKED) - } + if (grantResults.isNotEmpty() && allPermissionWereGranted) { + appPermissionRequest.grant() + } else { + appPermissionRequest.reject() + permissions.forEach { systemPermission -> + if (!onShouldShowRequestPermissionRationale(systemPermission)) { + // The system permission is denied permanently + storeSitePermissions( + currentContentSate, + appPermissionRequest, + status = BLOCKED + ) } } - consumeAppPermissionRequest(appPermissionRequest) } - true + consumeAppPermissionRequest(appPermissionRequest) + } + } + + private fun getCurrentContentState() = + store.state.findTabOrCustomTabOrSelectedTab(customTabId)?.content + + @VisibleForTesting + internal fun findRequestedAppPermission(permissions: Array): PermissionRequest? { + return getCurrentContentState()?.appPermissionRequestsList?.find { + permissions.contains(it.permissions.first().id) } } @@ -217,15 +229,15 @@ class SitePermissionsFeature( * If it's true none prompt will show otherwise the prompt will be shown. */ internal fun onContentPermissionGranted( - session: Session, permissionRequest: PermissionRequest, shouldStore: Boolean ) { permissionRequest.grant() if (shouldStore) { - storeSitePermissions(session, permissionRequest, ALLOWED) + getCurrentContentState()?.let { contentState -> + storeSitePermissions(contentState, permissionRequest, ALLOWED) + } } - consumePermissionRequest(permissionRequest) } internal fun onPositiveButtonPress( @@ -233,11 +245,8 @@ class SitePermissionsFeature( sessionId: String, shouldStore: Boolean ) { - sessionManager.runWithSession(sessionId) { session -> - consumePermissionRequest(permissionRequest) - onContentPermissionGranted(session, permissionRequest, shouldStore) - true - } + consumePermissionRequest(permissionRequest, sessionId) + onContentPermissionGranted(permissionRequest, shouldStore) } internal fun onNegativeButtonPress( @@ -245,47 +254,42 @@ class SitePermissionsFeature( sessionId: String, shouldStore: Boolean ) { - sessionManager.runWithSession(sessionId) { session -> - onContentPermissionDeny(permissionRequest, session, shouldStore) - consumePermissionRequest(permissionRequest) - true - } + consumePermissionRequest(permissionRequest, sessionId) + onContentPermissionDeny(permissionRequest, shouldStore) } internal fun onDismiss( permissionRequest: PermissionRequest, sessionId: String ) { - sessionManager.runWithSession(sessionId) { session -> - consumePermissionRequest(permissionRequest) - onContentPermissionDeny(permissionRequest, session, false) - true - } + consumePermissionRequest(permissionRequest, sessionId) + onContentPermissionDeny(permissionRequest, false) } internal fun storeSitePermissions( - session: Session, + contentState: ContentState, request: PermissionRequest, status: SitePermissions.Status ) { - if (session.private) { + if (contentState.private) { return } ioCoroutineScope.launch { synchronized(storage) { + val host = contentState.url.tryGetHostFromUrl() var sitePermissions = - storage.findSitePermissionsBy(request.getHost(session)) + storage.findSitePermissionsBy(host) if (sitePermissions == null) { sitePermissions = request.toSitePermissions( - session, + host, status = status, permissions = request.permissions ) storage.save(sitePermissions) } else { - sitePermissions = request.toSitePermissions(session, status, sitePermissions) + sitePermissions = request.toSitePermissions(host, status, sitePermissions) storage.update(sitePermissions) } } @@ -300,14 +304,14 @@ class SitePermissionsFeature( */ internal fun onContentPermissionDeny( permissionRequest: PermissionRequest, - session: Session, shouldStore: Boolean ) { permissionRequest.reject() if (shouldStore) { - storeSitePermissions(session, request = permissionRequest, status = BLOCKED) + getCurrentContentState()?.let { contentState -> + storeSitePermissions(contentState, permissionRequest, BLOCKED) + } } - consumePermissionRequest(permissionRequest) } internal suspend fun onContentPermissionRequested( @@ -418,9 +422,9 @@ class SitePermissionsFeature( } private fun PermissionRequest.toSitePermissions( - session: Session, + host: String, status: SitePermissions.Status, - initialSitePermission: SitePermissions = getInitialSitePermissions(session, this), + initialSitePermission: SitePermissions = getInitialSitePermissions(host/*, this*/), permissions: List = this.permissions ): SitePermissions { var sitePermissions = initialSitePermission @@ -431,15 +435,15 @@ class SitePermissionsFeature( } internal fun getInitialSitePermissions( - session: Session, - request: PermissionRequest + host: String/*, + request: PermissionRequest*/ ): SitePermissions { val rules = sitePermissionsRules return rules?.toSitePermissions( - request.getHost(session), + host, savedAt = System.currentTimeMillis() ) - ?: SitePermissions(request.getHost(session), savedAt = System.currentTimeMillis()) + ?: SitePermissions(host, savedAt = System.currentTimeMillis()) } private fun PermissionRequest.isForAutoplay() =