Audit Gecko's site permission to make sure GV asks the app instead of storing the permission decision internally

RESOLVED FIXED in Firefox 67

Status

defect
P1
normal
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: amejiamarmol, Assigned: fluffyemily)

Tracking

unspecified
mozilla67
Unspecified
Android

Firefox Tracking Flags

(firefox-esr60 wontfix, firefox65 wontfix, firefox66 wontfix, firefox67 fixed)

Details

(Whiteboard: [geckoview:fenix:m2], URL)

Attachments

(1 attachment)

(Reporter)

Description

3 months ago

After rejecting a PERMISSION_DESKTOP_NOTIFICATION, it is not possible to grant it, even though grant() is called on GeckoSession.PermissionDelegate.Callback.

You can reproduce it on GeckoView Sample App.

Steps to reproduce

  1. Request a Notification permission.
  2. Deny it.
  3. Request it again.
  4. Grant it.

Expected behavior

The permission change from denied to granted.

Actual behavior

The permission remains denied.

Yeah, I believe Gecko is storing the response and so we never send the request again. It was decided in bug 1524489 that we'd rather have GV ask the app every time rather than have some kind of site permission management API. I'll morph this bug into the work required for that.

Summary: After rejecting a PERMISSION_DESKTOP_NOTIFICATION it is not possible to grant it → GeckoView should not store site permission responses
(Reporter)

Comment 2

3 months ago

👍
Note: Be aware that the only permission that is behaving in this way in is PERMISSION_DESKTOP_NOTIFICATION, the other ones are doing a expected, they can be grant or reject without any issues.

Whiteboard: [geckoview:fenix:m2]
Priority: -- → P1
Summary: GeckoView should not store site permission responses → GeckoView should not store site permission responses (PERMISSION_DESKTOP_NOTIFICATION)

James says we should audit all site permissions in core Gecko.

Summary: GeckoView should not store site permission responses (PERMISSION_DESKTOP_NOTIFICATION) → Audit Gecko's site permission to make sure GV asks the app instead of storing the permission decision internally
(Assignee)

Updated

3 months ago
Assignee: nobody → etoop

I suppose this is within scope of this audit, but I want us to make sure that it's possible for different products to implement different logic on top of GV's permission system.
E.g. "deny all by default" (for Focus-like use cases), or using "Never" rather than "Not this time" (like Chrome's new notification permission doorhanger), allowing for more aggressive permission management.

(In reply to Andreas Bovens [:abovens] from comment #4)

I suppose this is within scope of this audit, but I want us to make sure that it's possible for different products to implement different logic on top of GV's permission system.
E.g. "deny all by default" (for Focus-like use cases), or using "Never" rather than "Not this time" (like Chrome's new notification permission doorhanger), allowing for more aggressive permission management.

Yeah, that's exactly what this bug is trying to enable. GV will always ask the app, and the app can then implement whatever policy it wishes (as you described above).

See Also: → 1528303

I was thinking about this some this morning because Nevin was asking about it, and I wonder if we might be in bad shape here. It looks like most of this is in nsPermissionManager, which implements nsIPermissionManager. There are several methods available to query the current permissions. If we use that for anything besides management UI in Firefox it'll probably be bad news, since it doesn't really makes sense for us to expose that to apps the way things stand today. Ideally we'd replace nsPermissionManager entirely, but I'm not sure how feasible that really is.

A-C plans to complete their site permissions API in M3, so GV should provide the GV API in M2.

https://github.com/mozilla-mobile/android-components/issues/1818

Depends on: 1527074
(Assignee)

Comment 8

3 months ago

After some investigation, we don't store the desktop permission response anywhere. It is permission.site that prevents the permission from being allowed afterwards.

The difference between the behaviour on GV and Fennec or Chrome (and Fx Desktop), is that once the permission is denied on the other browsers then requesting the permission again does not bring the notification permission dialog up again and simply returns the previous denied response, whereas on GV we re-present the request it, giving users the option to grant again, but then permission.site does not display that granted permission, even though it receives the grant.

What is actually happening is that the new css style is being appended to the styles, resulting in a style of "error success" or "success error". The "error" style overrides the "success" style which results in the style never changing once the permission has been denied. Desktop notifications are handled in their own special case, which is why this behavior does not happen for other permissions. I believe this is because the site does not expect that request to be reissued and overwritten.

Assuming that the behaviour we want is the same as for other browsers, I will look into what we need to do to not re-request after denial, but I suspect that this is something we want consumers to implement, and not GV itself? Do you have an opinion :snorp? The [Notification spec][1] itself does not specify what the expected behavior should be.

[1] https://notifications.spec.whatwg.org

Flags: needinfo?(snorp)

Whoops, sorry about the late reply. I think we always want GV to ask for the permission whenever content does, and leave it up to the app to decide policy around caching responses.

Flags: needinfo?(snorp)

Comment 11

2 months ago
Pushed by etoop@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0a192c810854
Update GVE and Documentation to reflect proper handling of Notification permissions. r=geckoview-reviewers,snorp

Comment 12

2 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.