Closed Bug 1369581 Opened 3 years ago Closed 3 years ago

Requesting only the "activeTab" permission fails

Categories

(WebExtensions :: General, defect, P2)

defect

Tracking

(firefox56 verified)

VERIFIED FIXED
mozilla56
Tracking Status
firefox56 --- verified

People

(Reporter: wbamberg, Assigned: bsilverberg)

Details

(Whiteboard: [permissions] triaged)

Attachments

(3 files)

Attached file perms.zip
I've attached an add-on. It has the following in optional_permissions: ["history", "activeTab"].

If you install it, then click the browser action, it opens a page with a button labeled "request". Clicking this button causes the add-on to ask for the "activeTab" permission only. This will fail, returning false without showing the dialog.

If you edit "const permissionsToRequest" to be ["history"], it asks for "history", and will grant it.

If you edit "const permissionsToRequest" to be ["history", "activeTab"], it asks for "history", and will grant both "history" and "activeTab".
I will look into this.
Assignee: nobody → bob.silverberg
Whiteboard: investigate
At first glance I notice that "activeTab" does not have a description for a permissions message in browser/locales/en-US/chrome/browser/browser.properties, so it's not expected to display a prompt (I think). That might explain your third case where it grants both "history" and "activeTab" but only prompts for "history". The first case, in which it doesn't grant the permission silently is something else though.
Flags: needinfo?(wbamberg)
Flags: needinfo?(aswan)
Oops, didn't mean to needinfo.
Flags: needinfo?(wbamberg)
Flags: needinfo?(aswan)
The problem is in ExtensionsUI.jsm at line 237 [1]. We should be calling `resolve(true)` rather than just `resolve()`. I have prepared a patch which I will attach to this bug. I wanted to add a test, and I started by adding a case into test_ext_permissions.js at [2], but then I realized that this test doesn't actually call the code in ExtensionsUI.jsm at all, so adding it there does nothing.

Do we want to add a test that tests this specific change, and if so do you have any suggestions for how to write such a test?

[1] http://searchfox.org/mozilla-central/source/browser/modules/ExtensionsUI.jsm#237
[2] http://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/xpcshell/test_ext_permissions.js#146
Flags: needinfo?(aswan)
Priority: -- → P2
Whiteboard: investigate → [permissions] triaged
Thank you for jumping on this so quickly!  The tests that exercise the front-end browser code are in browser/base/content/test/webextensions/  When they were originally written, they used signed xpis which was awkward.  In the old style, writing a test with new permissions would mean creating and signing a new xpi.  However, in the mean time Kris landed changes that let us load unsigned extensions in automation, so one of the cleanups I'd like to do is to rip out the signed xpis and generate them on the fly from the tests, this would also make the tests more readable.  browser_extension_sideloading.js in that directory already follows this pattern.  If you're up for doing the testing, I'd be happy with anything from an expedient change to cover this case all the way up to completely reworking the test to get rid of the signed xpis, just wanted to give you the background and you can decide.
Flags: needinfo?(aswan)
It seems odd to allow optional_permissions that we don't prompt for. Chrome doesn't allow activeTab in optional_permissions (perhaps because it's hard to present to the user), is there a reason we do?

Looking at http://searchfox.org/mozilla-central/source/browser/modules/ExtensionsUI.jsm#237, it seems like we will silently grant any permissions that don't have a description in chrome://browser/locale/browser.properties (http://searchfox.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.properties#98).

I think the list of possible API optional_permissions is:

activeTab
bookmarks
clipboardRead
clipboardWrite
cookies
geolocation
history
idle
notifications
tabs
topSites
webNavigation
webRequest
webRequestBlocking

Of these, the following don't have descriptions in chrome://browser/locale/browser.properties:

activeTab
cookies
idle
webRequest
webRequestBlocking

This means, with the current code, I can write this in my manifest.json:

"optional_permissions": [
    "tabs", "webRequest", "cookies", "idle", "activeTab", "webRequestBlocking"
  ]

I can then request all 6 permissions, and the browser will only ask the user about "tabs". If the user accepts, I get all 6. With this fix, AIUI, I can ask for ["webRequest", "cookies", "idle", "activeTab", "webRequestBlocking"], and get all 5, without any prompt. Is this the intended behavior?
Yes, it does seem a little quirky but we also don't prompt at install time if these are regular (non-optional) permissions.
For cookies, webRequest, and webRequestBlocking, the rationale is that those can only be used in conjunction with host permissions, and a user is probably much better equipped to decide on individual host permission (ie, "this extension can see all your data on somesite.com") than on whether the extension can use its host permissions to get at cookies/web requests/etc

I'm less clear on activeTab but the reason it exists is to give extensions that work with the current active tab a way to do so without having to ask for expansive permissions (ie <all_urls>).  And having a permission for idle is just weird, but since it is a permission, I think its best to grant it silently rather than trying to pose a comprehensible question to users to let them choose whether to grant it.
(In reply to Andrew Swan [:aswan] from comment #6)
> Thank you for jumping on this so quickly!  The tests that exercise the
> front-end browser code are in browser/base/content/test/webextensions/  When
> they were originally written, they used signed xpis which was awkward.  In
> the old style, writing a test with new permissions would mean creating and
> signing a new xpi.  However, in the mean time Kris landed changes that let
> us load unsigned extensions in automation, so one of the cleanups I'd like
> to do is to rip out the signed xpis and generate them on the fly from the
> tests, this would also make the tests more readable. 
> browser_extension_sideloading.js in that directory already follows this
> pattern.  If you're up for doing the testing, I'd be happy with anything
> from an expedient change to cover this case all the way up to completely
> reworking the test to get rid of the signed xpis, just wanted to give you
> the background and you can decide.

Thanks Andrew. I've taken a look at the tests in that folder, as well as what we're already doing for permissions in /toolkit tests, and I'm pretty confused about what route to take for this. I'll ping you later to see if we can have a chat about how best to approach this.
Comment on attachment 8873824 [details]
Bug 1369581 - Requesting an optional permission that does not cause a prompt should succeed,

https://reviewboard.mozilla.org/r/145250/#review149886

clearing review flag for now
Attachment #8873824 - Flags: review?(aswan)
Comment on attachment 8873824 [details]
Bug 1369581 - Requesting an optional permission that does not cause a prompt should succeed,

https://reviewboard.mozilla.org/r/145250/#review154666
Attachment #8873824 - Flags: review?(aswan) → review+
Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2cf23132f3b1
Requesting an optional permission that does not cause a prompt should succeed, r=aswan
https://hg.mozilla.org/mozilla-central/rev/2cf23132f3b1
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Attached image PermissionGranted.gif
I can reproduce this issue on Firefox 56.0a1 (20170619030208) on Wind 7 64-bit, the following message “Permission was refused” is displayed in the browser console.
 
This issue is verified as fixed on Firefox 56.0a1 (2017-06-20) on Wind 7 64-bit and Ubuntu 12.04 LTS 64-bit, the following message “Permission was granted” is displayed in the browser console, please have a look at the attached video for this fix.
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.