1788720 - Expose private browsing flag in StorageController.getPermissions

Status: RESOLVED FIXED

(GeckoView :: Core, task, P2)

Description

[Jonathan Almeida [:jonalmeida]]

While reviewing bug 1787034, we discovered that the current permissions API only allows for querying with the uri. However, the application would also need to know if the permission has been granted when in a private browsing context.

A quick search tells us we are already surfacing the private mode flag in ContentPermission, so we only require to surface the flag in the StorageController#getPermissions API.

Current API:
public GeckoResult<List<ContentPermission>> getPermissions(String uri)

Proposed API:
public GeckoResult<List<ContentPermission>> getPermissions(String uri, boolean privateMode)

---

For setting the permission, it's unclear to me if StorageController#setPrivateBrowsingPermanentPermission is enough for setting temporary permissions in PB mode, so setPermissions may also require API changes.

Comment 1

[Chris Peterson [:cpeterson]]

107

Comment 2

[Chris Peterson [:cpeterson]]

108

Comment 3

[Arturo Mejia [:amejia]]

Attachment: GeckoView.patch

For querying site permissions, we exposing a function that search for both private and normal mode sessions, that cause issues when users granted/denied permissions in normal mode but not in private mode as that permissions where leaked to private mode, now we are exposing a new function getPermissions(uri, privateMode) that force consumer to explicitly indicate in which mode the permissions should be, in addition to that we needed to exposed sessions mode (privateBrowsing) for notifications, this way we are also explicit when showing notifications for a site.

Comment 4

[Arturo Mejia [:amejia]]

Attachment: AC.patch

To follow with the changes on GeckoView, we need to propagate the changes into AC and apps

Comment 5

[Arturo Mejia [:amejia]]

Attachment: Fenix.patch

Comment 6

[Arturo Mejia [:amejia]]

Side note: Focus doesn't need any changes as it wasn't using the getPermissions(uri) api.

Comment 7

[Christian Sadilek [:csadilek]]

Comment on attachment 9299043 [details] [diff] [review]
Fenix.patch

Review of attachment 9299043 [details] [diff] [review]:
-----------------------------------------------------------------

::: app/src/main/java/org/mozilla/fenix/settings/sitepermissions/SitePermissionsDetailsExceptionsFragment.kt
@@ +61,5 @@
>              sitePermissions =
> +                requireNotNull(
> +                    requireComponents.core.permissionStorage.findSitePermissionsBy(
> +                        sitePermissions.origin,
> +                        private = false,

In the UI in Fenix, shouldn't we take the current browsing mode into account? Otherwise when this lands we'd (incorrectly?):

- Show exceptions in private browsing that don't apply (exceptions/permissions stored in a regular browsing session).
- Not show exceptions added in the current private browsing session.

::: app/src/main/java/org/mozilla/fenix/settings/sitepermissions/SitePermissionsManageExceptionsPhoneFeatureFragment.kt
@@ +225,5 @@
>          val updatedSitePermissions = getSitePermission().update(getFeature(), status)
>          viewLifecycleOwner.lifecycleScope.launch(Main) {
> +            requireComponents.core.permissionStorage.updateSitePermissions(
> +                sitePermissions = updatedSitePermissions,
> +                private = false,

See my q above in SitePermissionsDetailsExceptionsFragment.kt .

@@ +237,5 @@
>          val updatedSitePermissions = autoplayValue.updateSitePermissions(getSitePermission())
>          viewLifecycleOwner.lifecycleScope.launch(Main) {
> +            requireComponents.core.permissionStorage.updateSitePermissions(
> +                sitePermissions = updatedSitePermissions,
> +                private = false,

See my q above in SitePermissionsDetailsExceptionsFragment.kt

Comment 8

[Christian Sadilek [:csadilek]]

Comment on attachment 9299042 [details] [diff] [review]
AC.patch

Review of attachment 9299042 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me! I had one question inline.

::: components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/permission/GeckoSitePermissionsStorage.kt
@@ +104,4 @@
>  
> +    internal fun SitePermissions?.resetNoGeckoPermissionIfNeeded(private: Boolean): SitePermissions? {
> +        return if (private) {
> +            this?.copy(camera = NO_DECISION, microphone = NO_DECISION, bluetooth = NO_DECISION)

Why do we not need to reset other permission types e.g., autoplay?

Comment 9

[Christian Sadilek [:csadilek]]

Comment on attachment 9299040 [details] [diff] [review]
GeckoView.patch

Review of attachment 9299040 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!

Comment 10

[Arturo Mejia [:amejia]]

Attachment: Site_permissions_Exceptions.png

Comment 11

[Arturo Mejia [:amejia]]

::: app/src/main/java/org/mozilla/fenix/settings/sitepermissions/SitePermissionsDetailsExceptionsFragment.kt @@ +61,5 @@
In the UI in Fenix, shouldn't we take the current browsing mode into account? Otherwise when this lands we'd (incorrectly?):

This class correspond to the screen where users manage stored permissions (Settings -> Site permissions -> Exceptions -> A site), as we only store normal permission sessions, we can assume that only normal sessions are handle here :)

Comment 12

[Arturo Mejia [:amejia]]

Attachment: feature_phone_site_permission.png

Comment 13

[Arturo Mejia [:amejia]]

::: app/src/main/java/org/mozilla/fenix/settings/sitepermissions/SitePermissionsManageExceptionsPhoneFeatureFragment.kt
@@ +225,5 @@
val updatedSitePermissions = getSitePermission().update(getFeature(), status)
viewLifecycleOwner.lifecycleScope.launch(Main) {

•        requireComponents.core.permissionStorage.updateSitePermissions(
  

•            sitePermissions = updatedSitePermissions,
  

•            private = false,
  

See my q above in SitePermissionsDetailsExceptionsFragment.kt .

Same as comment 11, this is a sub screen of where users manage stored permissions (Settings -> Site permissions -> Exceptions -> A site -> A permission of the site). As we don't store private permissions, it's safe to assume that users can only alter normal permissions in this screen.

Comment 14

[Arturo Mejia [:amejia]]

@@ +237,5 @@
val updatedSitePermissions = autoplayValue.updateSitePermissions(getSitePermission())
viewLifecycleOwner.lifecycleScope.launch(Main) {

•        requireComponents.core.permissionStorage.updateSitePermissions(
  

•            sitePermissions = updatedSitePermissions,
  

•            private = false,
  

See my q above in SitePermissionsDetailsExceptionsFragment.kt

Same as comment 13.

Sorry for not clarifying these points as part of submit the review.

Comment 15

[Arturo Mejia [:amejia]]

Looks good to me! I had one question inline.

::: components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/permission/GeckoSitePermissionsStorage.kt
@@ +104,4 @@

• internal fun SitePermissions?.resetNoGeckoPermissionIfNeeded(private: Boolean): SitePermissions? {

•    return if (private) {
  

•        this?.copy(camera = NO_DECISION, microphone = NO_DECISION, bluetooth = NO_DECISION)
  

Why do we not need to reset other permission types e.g., autoplay?

These other permissions are stored directly by gecko and it knows the right browsing mode, permissions like camera, microphone and bluetooth are stored in AC. A longer explanation can be find {here in bug 1796614](https://bugzilla.mozilla.org/show_bug.cgi?id=1796614) , as I filed it to improve our the documentation related to permission, as unfortunately it's not straightforward after the site permissions migration.

Comment 16

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 9299040 [details] [diff] [review]
GeckoView.patch

Deferring my r? to csadilek's review.

Comment 17

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 9299042 [details] [diff] [review]
AC.patch

Deferring my r? to csadilek's review.

Comment 18

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 9299043 [details] [diff] [review]
Fenix.patch

Deferring my r? to csadilek's review.

Comment 19

[Christian Sadilek [:csadilek]]

These other permissions are stored directly by gecko and it knows the right browsing mode, permissions like camera, microphone and bluetooth are stored in AC.

OK, let's add this (and the link to the ticket) as comments to the code, please :).

Same as comment 11, this is a sub screen of where users manage stored permissions (Settings -> Site permissions -> Exceptions -> A site -> A permission of the site). As we don't store private permissions, it's safe to assume that users can only alter normal permissions in this screen.

Right, but essentially, it displays permissions that are not accurate in private browsing mode, which is likely confusing? Since this is already the case though, we can handle in a follow-up / separate ticket.

Comment 20

[Arturo Mejia [:amejia]]

Attachment: Bug 1788720 - Expose private browsing flag in StorageController.getPermissions

Depends on D159632

Comment 21

[Arturo Mejia [:amejia]]

(In reply to Christian Sadilek [:csadilek] from comment #19)

These other permissions are stored directly by gecko and it knows the right browsing mode, permissions like camera, microphone and bluetooth are stored in AC.

OK, let's add this (and the link to the ticket) as comments to the code, please :).

Same as comment 11, this is a sub screen of where users manage stored permissions (Settings -> Site permissions -> Exceptions -> A site -> A permission of the site). As we don't store private permissions, it's safe to assume that users can only alter normal permissions in this screen.

Right, but essentially, it displays permissions that are not accurate in private browsing mode, which is likely confusing? Since this is already the case though, we can handle in a follow-up / separate ticket.

I filed bug #1797505 to follow-up on the theme of the settings page also tagged Nicole to see if she has more context around it.

Comment 22

[Arturo Mejia [:amejia]]

Attachment: Bug 1788720 - Expose private browsing flag in StorageController.getPermissions

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Landed: https://hg.mozilla.org/integration/autoland/rev/9cda62240050be7330f01c612d6c242c5c3026ae

Backed out for linting failure: https://hg.mozilla.org/integration/autoland/rev/18edd8ea36a9afb7604fe19500fd64576256f2c1

Push with failure: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=RTFSnSe6RKSyXMZuBWWWdQ.0&resultStatus=retry%2Ctestfailed%2Cbusted%2Cexception%2Cusercancel&revision=9cda62240050be7330f01c612d6c242c5c3026ae
Failure log: https://treeherder.mozilla.org/logviewer?job_id=394464302&repo=autoland
Complains about StorageController.java and WebNotification.java.

Comment 24

[Arturo Mejia [:amejia]]

Attachment: Bug 1788720 - Add getPermissions to geckoView CHANGELOG.md

Depends on D160424

Comment 25

[Arturo Mejia [:amejia]]

I added https://phabricator.services.mozilla.com/D160444 to address the issues.

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Landed: https://hg.mozilla.org/integration/autoland/rev/c7074f4cf73c0649b066fa7d2d171e72f099bab2

Backed out for failing two gv-junit tests: https://hg.mozilla.org/integration/autoland/rev/9bae09f9ac8274df1fc8270679d9a25085655636
Failure log: https://treeherder.mozilla.org/logviewer?job_id=394736845&repo=autoland

TEST-UNEXPECTED-FAIL | org.mozilla.geckoview.test.PermissionDelegateTest#contextId | java.lang.AssertionError: Notification permission should be set to allow
TEST-UNEXPECTED-FAIL | org.mozilla.geckoview.test.WebNotificationTest#clickNotificationParceled | java.lang.AssertionError: privateBrowsing should match

Comment 27

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Landed: https://hg.mozilla.org/integration/autoland/rev/6d8714cc305a52d2656fc44bc89149a42c6449a2

Backed out for https://hg.mozilla.org/integration/autoland/rev/e717b836ceaaf32bb2e1a6d0c3af788011629314
Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=6d8714cc305a52d2656fc44bc89149a42c6449a2&searchStr=geckoview&selectedTaskRun=LucgGyZESmeMH_mVt2J8yw.0
Failure log: https://treeherder.mozilla.org/logviewer?job_id=395128658&repo=autoland
Failure line: TEST-UNEXPECTED-FAIL | org.mozilla.geckoview.test.WebNotificationTest#clickNotificationParceled | java.lang.AssertionError: privateBrowsing should match

Please test the patch on Try before landing it again.

Comment 28

[Arturo Mejia [:amejia]]

Sorry for the inconveniences.
This is the try link.
https://treeherder.mozilla.org/jobs?repo=try&revision=35bf62c6a701a4bbd425ae5ae6b57ad81748f992

On it t

The tests that are failing are already reported as interment and seem to be unrelated to this change.

https://bugzilla.mozilla.org/buglist.cgi?quicksearch=test_popup_button.xhtml&list_id=16271970
https://bugzilla.mozilla.org/show_bug.cgi?id=1775915
https://bugzilla.mozilla.org/show_bug.cgi?id=1777035

Comment 29

[Arturo Mejia [:amejia]]

I updated https://phabricator.services.mozilla.com/D160424 addressing comment 27.
I'll try again to land as locally tests and on the try server failures are already addressed.

Comment 30

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Expose private browsing flag in StorageController.getPermissions r=geckoview-reviewers,jonalmeida
https://hg.mozilla.org/integration/autoland/rev/3a4b59be9d361c2e819d08e100fcc6e890fde0e9
https://hg.mozilla.org/mozilla-central/rev/3a4b59be9d36

Comment 31

[Arturo Mejia [:amejia]]

Thanks for all the help Sebastian!