From d665a3de904aaa8df5faf8fb343c704f97d4b849 Mon Sep 17 00:00:00 2001 From: Arturo Mejia Date: Fri, 11 Sep 2020 15:40:57 -0400 Subject: [PATCH] Closes #8323: Add-on permission dialog does not prompt for host permissions --- .../components/feature/addons/Addon.kt | 123 ++++++++++- .../addons/ui/PermissionsDialogFragment.kt | 5 +- .../feature/addons/update/AddonUpdater.kt | 6 +- .../addons/src/main/res/values/strings.xml | 12 + .../feature/addons/src/test/java/AddonTest.kt | 205 +++++++++++++++++- docs/changelog.md | 4 + .../samples/browser/DefaultComponents.kt | 2 +- .../addons/PermissionsDetailsActivity.kt | 4 +- 8 files changed, 343 insertions(+), 18 deletions(-) diff --git a/components/feature/addons/src/main/java/mozilla/components/feature/addons/Addon.kt b/components/feature/addons/src/main/java/mozilla/components/feature/addons/Addon.kt index 01a4a2de018..7710261b048 100644 --- a/components/feature/addons/src/main/java/mozilla/components/feature/addons/Addon.kt +++ b/components/feature/addons/src/main/java/mozilla/components/feature/addons/Addon.kt @@ -4,7 +4,9 @@ package mozilla.components.feature.addons +import android.content.Context import android.os.Parcelable +import androidx.core.net.toUri import kotlinx.android.parcel.Parcelize /** @@ -107,12 +109,11 @@ data class Addon( ) : Parcelable /** - * Returns a list of id resources per each item on the [permissions] list. + * Returns a list of localized Strings per each item on the [permissions] list. */ - fun translatePermissions(): List { - return localizePermissions(permissions) + fun translatePermissions(context: Context): List { + return localizePermissions(permissions, context) } - /** * Returns whether or not this [Addon] is currently installed. */ @@ -193,8 +194,118 @@ data class Addon( * @param permissions The list of permissions to be localized. Valid permissions can be found in * https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/permissions#API_permissions */ - fun localizePermissions(permissions: List): List { - return permissions.mapNotNull { permissionToTranslation[it] } + fun localizePermissions(permissions: List, context: Context): List { + var localizedUrlAccessPermissions = emptyList() + val requireAllUrlsAccess = permissions.contains("") + val notFoundPermissions = mutableListOf() + + val localizedNormalPermissions = permissions.mapNotNull { + val id = permissionToTranslation[it] + if (id == null) notFoundPermissions.add(it) + id + }.map { context.getString(it) } + + if (!requireAllUrlsAccess && notFoundPermissions.isNotEmpty()) { + localizedUrlAccessPermissions = localizedURLAccessPermissions(context, notFoundPermissions) + } + + return localizedNormalPermissions + localizedUrlAccessPermissions + } + + @Suppress("MaxLineLength") + internal fun localizedURLAccessPermissions(context: Context, accessPermissions: List): List { + val localizedSiteAccessPermissions = mutableListOf() + val permissionsToTranslations = mutableMapOf() + + accessPermissions.forEach { permission -> + val id = localizeURLAccessPermission(permission) + if (id != null) { + permissionsToTranslations[permission] = id + } + } + + if (permissionsToTranslations.values.any { it.isAllURLsPermission() }) { + localizedSiteAccessPermissions.add(context.getString(R.string.mozac_feature_addons_permissions_all_urls_description)) + } else { + formatURLAccessPermission(permissionsToTranslations, localizedSiteAccessPermissions, context) + } + return localizedSiteAccessPermissions + } + + @Suppress("MagicNumber", "ComplexMethod") + private fun formatURLAccessPermission( + permissionsToTranslations: MutableMap, + localizedSiteAccessPermissions: MutableList, + context: Context + ) { + var domainCount = 0 + var siteCount = 0 + + loop@ for ((permission, stringId) in permissionsToTranslations) { + var host = permission.toUri().host ?: "" + when { + stringId.isDomainAccessPermission() -> { + ++domainCount + host = host.removePrefix("*.") + + if (domainCount > 4) continue@loop + } + + stringId.isSiteAccessPermission() -> { + ++siteCount + if (siteCount > 4) continue@loop + } + } + localizedSiteAccessPermissions.add(context.getString(stringId, host)) + } + + // If we have 4 or fewer permissions, display them all, otherwise we + // display the first 4 followed by an item that says "...plus N others" + if (domainCount > 4) { + val collapsedPermissions = domainCount - 4 + val string = if (collapsedPermissions > 1) { + R.string.mozac_feature_addons_permissions_to_many_domains_description_plural + } else { + R.string.mozac_feature_addons_permissions_to_many_domains_description + } + localizedSiteAccessPermissions.add(context.getString(string, collapsedPermissions)) + } + if (siteCount > 4) { + val collapsedPermissions = siteCount - 4 + val string = if (collapsedPermissions > 1) { + R.string.mozac_feature_addons_permissions_to_many_sites_description_plural + } else { + R.string.mozac_feature_addons_permissions_to_many_sites_description + } + localizedSiteAccessPermissions.add(context.getString(string, collapsedPermissions)) + } + } + + private fun Int.isSiteAccessPermission(): Boolean { + return this == R.string.mozac_feature_addons_permissions_one_site_description + } + + private fun Int.isDomainAccessPermission(): Boolean { + return this == R.string.mozac_feature_addons_permissions_sites_in_domain_description + } + + private fun Int.isAllURLsPermission(): Boolean { + return this == R.string.mozac_feature_addons_permissions_all_urls_description + } + + internal fun localizeURLAccessPermission(urlAccess: String): Int? { + val uri = urlAccess.toUri() + val host = (uri.host ?: "").trim() + val path = (uri.path ?: "").trim() + + return when { + host == "*" || urlAccess == "" -> { + R.string.mozac_feature_addons_permissions_all_urls_description + } + host.isEmpty() || path.isEmpty() -> null + host.startsWith(prefix = "*.") -> R.string.mozac_feature_addons_permissions_sites_in_domain_description + else -> R.string.mozac_feature_addons_permissions_one_site_description + } } /** diff --git a/components/feature/addons/src/main/java/mozilla/components/feature/addons/ui/PermissionsDialogFragment.kt b/components/feature/addons/src/main/java/mozilla/components/feature/addons/ui/PermissionsDialogFragment.kt index 9855094298c..7a8ac323aa4 100644 --- a/components/feature/addons/src/main/java/mozilla/components/feature/addons/ui/PermissionsDialogFragment.kt +++ b/components/feature/addons/src/main/java/mozilla/components/feature/addons/ui/PermissionsDialogFragment.kt @@ -183,12 +183,11 @@ class PermissionsDialogFragment : AppCompatDialogFragment() { internal fun buildPermissionsText(): String { var permissionsText = getString(R.string.mozac_feature_addons_permissions_dialog_subtitle) + "\n\n" - val permissions = addon.translatePermissions() + val permissions = addon.translatePermissions(requireContext()) permissions.forEachIndexed { index, item -> val brakeLine = if (index + 1 != permissions.size) "\n\n" else "" - val permissionText = requireContext().getString(item) - permissionsText += "• $permissionText $brakeLine" + permissionsText += "• $item $brakeLine" } return permissionsText } diff --git a/components/feature/addons/src/main/java/mozilla/components/feature/addons/update/AddonUpdater.kt b/components/feature/addons/src/main/java/mozilla/components/feature/addons/update/AddonUpdater.kt index 4917eb109da..958ebf017e6 100644 --- a/components/feature/addons/src/main/java/mozilla/components/feature/addons/update/AddonUpdater.kt +++ b/components/feature/addons/src/main/java/mozilla/components/feature/addons/update/AddonUpdater.kt @@ -207,7 +207,7 @@ class DefaultAddonUpdater( ) { logger.info("onUpdatePermissionRequest $current") - val shouldGrantWithoutPrompt = Addon.localizePermissions(newPermissions).isEmpty() + val shouldGrantWithoutPrompt = Addon.localizePermissions(newPermissions, applicationContext).isEmpty() val shouldShowNotification = updateStatusStorage.isPreviouslyAllowed(applicationContext, updated.id) || shouldGrantWithoutPrompt @@ -306,7 +306,7 @@ class DefaultAddonUpdater( @VisibleForTesting internal fun createContentText(newPermissions: List): String { - val validNewPermissions = Addon.localizePermissions(newPermissions) + val validNewPermissions = Addon.localizePermissions(newPermissions, applicationContext) val string = if (validNewPermissions.size == 1) { R.string.mozac_feature_addons_updater_notification_content_singular @@ -317,7 +317,7 @@ class DefaultAddonUpdater( var permissionIndex = 1 val permissionsText = validNewPermissions.joinToString(separator = "\n") { - "${permissionIndex++}-${applicationContext.getString(it)}" + "${permissionIndex++}-$it" } return "$contentText:\n $permissionsText" } diff --git a/components/feature/addons/src/main/res/values/strings.xml b/components/feature/addons/src/main/res/values/strings.xml index a8bcb977f51..80579ed09db 100644 --- a/components/feature/addons/src/main/res/values/strings.xml +++ b/components/feature/addons/src/main/res/values/strings.xml @@ -7,6 +7,18 @@ Read and modify privacy settings Access your data for all websites + + Access your data for %1$s + + Access your data for sites in the %1$s domain + + Access your data on %1$d other site + + Access your data on %1$d other sites + + Access your data on %1$d other domain + + Access your data on %1$d other domains Access browser tabs diff --git a/components/feature/addons/src/test/java/AddonTest.kt b/components/feature/addons/src/test/java/AddonTest.kt index ee3b6858234..3a3285deb5c 100644 --- a/components/feature/addons/src/test/java/AddonTest.kt +++ b/components/feature/addons/src/test/java/AddonTest.kt @@ -7,6 +7,7 @@ package mozilla.components.feature.addons.amo import androidx.test.ext.junit.runners.AndroidJUnit4 import mozilla.components.feature.addons.Addon import mozilla.components.feature.addons.R +import mozilla.components.support.test.robolectric.testContext import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse import org.junit.Assert.assertTrue @@ -53,7 +54,7 @@ class AddonTest { updatedAt = "" ) - val translatedPermissions = addon.translatePermissions() + val translatedPermissions = addon.translatePermissions(testContext) val expectedPermissions = listOf( R.string.mozac_feature_addons_permissions_bookmarks_description, R.string.mozac_feature_addons_permissions_browser_setting_description, @@ -78,7 +79,7 @@ class AddonTest { R.string.mozac_feature_addons_permissions_unlimited_storage_description, R.string.mozac_feature_addons_permissions_web_navigation_description, R.string.mozac_feature_addons_permissions_top_sites_description - ) + ).map { testContext.getString(it) } assertEquals(expectedPermissions, translatedPermissions) } @@ -146,4 +147,204 @@ class AddonTest { assertTrue(addonEs.translatableName.contains(addonEn.defaultLocale)) assertTrue(addonEs.translatableName.contains("es")) } + + @Test + fun `localizedURLAccessPermissions - must translate all_urls access permission`() { + val expectedString = testContext.getString(R.string.mozac_feature_addons_permissions_all_urls_description) + val permissions = listOf( + "webRequest", + "webRequestBlocking", + "" + ) + + val result = Addon.localizedURLAccessPermissions(testContext, permissions).first() + + assertEquals(expectedString, result) + } + + @Test + fun `localizedURLAccessPermissions - must translate all urls access permission`() { + val expectedString = testContext.getString(R.string.mozac_feature_addons_permissions_all_urls_description) + val permissions = listOf( + "webRequest", + "webRequestBlocking", + "*://*/*" + ) + + val result = Addon.localizedURLAccessPermissions(testContext, permissions).first() + + assertEquals(expectedString, result) + } + + @Test + fun `localizedURLAccessPermissions - must translate domain access permissions`() { + val expectedString = listOf("tweetdeck.twitter.com", "twitter.com").map { + testContext.getString(R.string.mozac_feature_addons_permissions_one_site_description, it) + } + testContext.getString(R.string.mozac_feature_addons_permissions_all_urls_description) + val permissions = listOf( + "webRequest", + "webRequestBlocking", + "*://tweetdeck.twitter.com/*", + "*://twitter.com/*" + ) + + val result = Addon.localizedURLAccessPermissions(testContext, permissions) + + assertEquals(expectedString, result) + } + + @Test + fun `localizedURLAccessPermissions - must translate one site access permissions`() { + val expectedString = listOf("youtube.com", "youtube-nocookie.com", "vimeo.com").map { + testContext.getString(R.string.mozac_feature_addons_permissions_sites_in_domain_description, it) + } + testContext.getString(R.string.mozac_feature_addons_permissions_all_urls_description) + val permissions = listOf( + "webRequest", + "*://*.youtube.com/*", + "*://*.youtube-nocookie.com/*", + "*://*.vimeo.com/*" + ) + + val result = Addon.localizedURLAccessPermissions(testContext, permissions) + + assertEquals(expectedString, result) + } + + @Test + fun `localizedURLAccessPermissions - must collapse url access permissions`() { + var expectedString = listOf("youtube.com", "youtube-nocookie.com", "vimeo.com", "google.co.ao").map { + testContext.getString(R.string.mozac_feature_addons_permissions_sites_in_domain_description, it) + } + testContext.getString(R.string.mozac_feature_addons_permissions_to_many_domains_description, 1) + + var permissions = listOf( + "webRequest", + "*://*.youtube.com/*", + "*://*.youtube-nocookie.com/*", + "*://*.vimeo.com/*", + "*://*.google.co.ao/*", + "*://*.google.com.do/*" + ) + + var result = Addon.localizedURLAccessPermissions(testContext, permissions) + + // 1 domain permissions must be collapsed + assertEquals(expectedString, result) + + expectedString = listOf("youtube.com", "youtube-nocookie.com", "vimeo.com", "google.co.ao").map { + testContext.getString(R.string.mozac_feature_addons_permissions_sites_in_domain_description, it) + } + testContext.getString(R.string.mozac_feature_addons_permissions_to_many_domains_description_plural, 2) + + permissions = listOf( + "webRequest", + "*://*.youtube.com/*", + "*://*.youtube-nocookie.com/*", + "*://*.vimeo.com/*", + "*://*.google.co.ao/*", + "*://*.google.com.do/*", + "*://*.google.co.ar/*" + ) + + result = Addon.localizedURLAccessPermissions(testContext, permissions) + + // 2 domain permissions must be collapsed + assertEquals(expectedString, result) + + permissions = listOf( + "webRequest", + "*://www.youtube.com/*", + "*://www.youtube-nocookie.com/*", + "*://www.vimeo.com/*", + "https://mozilla.org/a/b/c/", + "*://www.google.com.do/*" + ) + + expectedString = listOf("www.youtube.com", "www.youtube-nocookie.com", "www.vimeo.com", "mozilla.org").map { + testContext.getString(R.string.mozac_feature_addons_permissions_one_site_description, it) + } + testContext.getString(R.string.mozac_feature_addons_permissions_to_many_sites_description, 1) + + result = Addon.localizedURLAccessPermissions(testContext, permissions) + + // 1 site permissions must be Collapsed + assertEquals(expectedString, result) + + permissions = listOf( + "webRequest", + "*://www.youtube.com/*", + "*://www.youtube-nocookie.com/*", + "*://www.vimeo.com/*", + "https://mozilla.org/a/b/c/", + "*://www.google.com.do/*", + "*://www.google.co.ar/*" + ) + + expectedString = listOf("www.youtube.com", "www.youtube-nocookie.com", "www.vimeo.com", "mozilla.org").map { + testContext.getString(R.string.mozac_feature_addons_permissions_one_site_description, it) + } + testContext.getString(R.string.mozac_feature_addons_permissions_to_many_sites_description_plural, 2) + + result = Addon.localizedURLAccessPermissions(testContext, permissions) + + // 2 site permissions must be Collapsed + assertEquals(expectedString, result) + + permissions = listOf( + "webRequest", + "*://www.youtube.com/*", + "*://www.youtube-nocookie.com/*", + "*://www.vimeo.com/*", + "https://mozilla.org/a/b/c/" + ) + + expectedString = listOf("www.youtube.com", "www.youtube-nocookie.com", "www.vimeo.com", "mozilla.org").map { + testContext.getString(R.string.mozac_feature_addons_permissions_one_site_description, it) + } + + result = Addon.localizedURLAccessPermissions(testContext, permissions) + + // None permissions must be collapsed + assertEquals(expectedString, result) + } + + @Test + fun `localizeURLAccessPermission - must provide the correct localized string`() { + val siteAccess = listOf( + "*://twitter.com/*", + "*://tweetdeck.twitter.com/*", + "https://mozilla.org/a/b/c/", + "https://www.google.com.ag/*", + "https://www.google.co.ck/*" + ) + + siteAccess.forEach { + val stringId = Addon.localizeURLAccessPermission(it) + assertEquals(R.string.mozac_feature_addons_permissions_one_site_description, stringId) + } + + val domainAccess = listOf( + "*://*.youtube.com/*", + "*://*.youtube.com/*", + "*://*.youtube-nocookie.com/*", + "*://*.vimeo.com/*", + "*://*.facebookcorewwwi.onion/*" + ) + + domainAccess.forEach { + val stringId = Addon.localizeURLAccessPermission(it) + assertEquals(R.string.mozac_feature_addons_permissions_sites_in_domain_description, stringId) + } + + val allUrlsAccess = listOf( + "*://*/*", + "http://*/*", + "https://*/*", + "file://*/*", + "" + ) + + allUrlsAccess.forEach { + val stringId = Addon.localizeURLAccessPermission(it) + assertEquals(R.string.mozac_feature_addons_permissions_all_urls_description, stringId) + } + } } \ No newline at end of file diff --git a/docs/changelog.md b/docs/changelog.md index 183ceae405b..b0c02733051 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -12,6 +12,10 @@ permalink: /changelog/ * [Gecko](/~https://github.com/mozilla-mobile/android-components/blob/master/buildSrc/src/main/java/Gecko.kt) * [Configuration](/~https://github.com/mozilla-mobile/android-components/blob/master/buildSrc/src/main/java/Config.kt) +* **feature-addons** + * ⚠️ **This is a breaking change**: The `Addon.translatePermissions` now requires a `context` object and returns a list of localized strings instead of a list of id string resources. + * 🚒 Bug [issue #8323](/~https://github.com/mozilla-mobile/android-components/issues/8323) Add-on permission dialog does not prompt for host permissions. + * **feature-recentlyclosed** * Added a new [RecentlyClosedTabsStorage] and a [RecentlyClosedMiddleware] to maintain a list of restorable recently closed tabs. diff --git a/samples/browser/src/main/java/org/mozilla/samples/browser/DefaultComponents.kt b/samples/browser/src/main/java/org/mozilla/samples/browser/DefaultComponents.kt index 2a2ddcd9911..c4d9d960bf5 100644 --- a/samples/browser/src/main/java/org/mozilla/samples/browser/DefaultComponents.kt +++ b/samples/browser/src/main/java/org/mozilla/samples/browser/DefaultComponents.kt @@ -177,7 +177,7 @@ open class DefaultComponents(private val applicationContext: Context) { AddonCollectionProvider( applicationContext, client, - collectionName = "3204bb44a6ef44d39ee34917f28055", + collectionName = "83a9cccfe6e24a34bd7b155ff9ee32", maxCacheAgeInMinutes = DAY_IN_MINUTES ) } diff --git a/samples/browser/src/main/java/org/mozilla/samples/browser/addons/PermissionsDetailsActivity.kt b/samples/browser/src/main/java/org/mozilla/samples/browser/addons/PermissionsDetailsActivity.kt index b26a483993e..c5148c7dac5 100644 --- a/samples/browser/src/main/java/org/mozilla/samples/browser/addons/PermissionsDetailsActivity.kt +++ b/samples/browser/src/main/java/org/mozilla/samples/browser/addons/PermissionsDetailsActivity.kt @@ -37,9 +37,7 @@ class PermissionsDetailsActivity : AppCompatActivity(), View.OnClickListener { private fun bindPermissions(addon: Addon) { val recyclerView = findViewById(R.id.add_ons_permissions) recyclerView.layoutManager = LinearLayoutManager(this) - val sortedPermissions = addon.translatePermissions().map { stringId -> - getString(stringId) - }.sorted() + val sortedPermissions = addon.translatePermissions(this).sorted() recyclerView.adapter = AddonPermissionsAdapter(sortedPermissions) }