1589896 - geolocation permission doesn't work when pref permissions.default.geo is non-zero

Status: ASSIGNED

(WebExtensions :: General, defect, P3)

Description

[Rob Wu [:robwu]]

I was looking into integrating the "notifications" permission in the permission manager (bug 1589693), and looked at our existing "geolocation" permission, since it has similar characteristics.

Upon reviewing the relevant code, it is obvious that the implementation has a flaw. It seems that the intention is to only add the permission if the user did not explicitly set it (e.g. to deny) (testPermission("geo") === Services.perms.UNKNOWN_ACTION).
However, when the permissions.default.geo preference is set, then that value is returned instead of UNKNOWN_ACTION, and the logic falls apart (i.e. permission is not added).

In order to fix this, we need to either be able to read the permission (ignoring the default pref branch), or be able to add a preference that overrides the default, but can be overridden by the user's explicit choice. The advantage of a system like the latter is that when the user resets the pref's value, that the extension permission is used as a fallback instead of the browser's default (which would happen anyway when the extension or browser is restarted).

Comment 1

[Johann Hofmann [:johannh]]

I would recommend just skipping the default pref check for WebExtension principals. That seems easiest to me, if I'm not missing something.

Comment 2

[Rob Wu [:robwu]]

I'm working on a patch where I change testPermission's implementation by replacing Services.perms.testPermissionFromPrincipal with Services.perms.getPermissionObject . This will achieve the desired effect, while the usual permission management logic is still consistent with regular http(s) web pages.

Comment 3

[Rob Wu [:robwu]]

Attachment: Bug 1589896 - Ignore permissions.default.* when applying extension permissions

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:jimm, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:robwu, could you have a look please?
For more information, please visit auto_nag documentation.