Open Bug 1398833 Opened 7 years ago Updated 2 years ago

chrome.permissions.request needs to be called directly from input handler, making it impossible to check for permissions first

Categories

(WebExtensions :: Compatibility, defect, P3)

Product:
WebExtensions
Component:
Compatibility
Version:
55 Branch
Type:
defect

Tracking

(Not tracked)

People

(Reporter: aleks.dimitrov, Unassigned)

References

Details

(Whiteboard: [permissons])

User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0 Build ID: 20170901071738 Steps to reproduce: Called from the options UI of a webextension, the following code works in Chrome, but not in Firefox: document.querySelector('#some-element').addEventListener('click', () => { const url = 'https://example.com'; chrome.permissions.contains( { origins: [url] }, (result) => { if (!result) { chrome.permissions.request( { origins: [url] }, (granted) => { if (granted) { console.log('granted permission'); ); } }, ); } }); }); It should be enough to stick this code in an HTML file, which has a <a id="some-element">click me</a>, and point the manifest to the file: "options_ui": { "page": "options.html" } Actual results: Clicking on #some-element yields the error message: Unchecked lastError value: Error: May only request permissions from a user input handler Calling chrome.permissions.request(...) directly from the handler and not putting it in a callback first works. Expected results: I'm unsure about the security implications, but the current model disallows requesting permissions if the decision to request them depends on a callback or promise. Chrome seems to have taken a different approach.
Component: Untriaged → WebExtensions: Compatibility
Product: Firefox → Toolkit
In this particular case, you don't actually need to check for the permission before requesting it, request() will just quietly return true if you request a permission you already have. From your description, it sounds as if Chrome is propagating the "isHandlingUserInput" state across some asynchronous operations. Rob, do you know exactly what Chrome does here and do you think its worth doing the same? Note that we have a handful of API methods that check this bit: http://searchfox.org/mozilla-central/search?q=requireUserInput&case=false&regexp=false&path=.json%24
Flags: needinfo?(rob)
(In reply to Andrew Swan [:aswan] from comment #1) > Rob, do you > know exactly what Chrome does here and do you think its worth doing the > same? Note that we have a handful of API methods that check this bit: > http://searchfox.org/mozilla-central/ > search?q=requireUserInput&case=false&regexp=false&path=.json%24 Chrome propagates the user input flag across callbacks, ever since the patch for https://crbug.com/361116 landed. It can make sense for us to do so too. I'm not sure whether it is technically feasible to implement this for our promise-based API though. With callbacks, the desired lifetime of user gestures is obvious (from the start of the callback until return, consumed when an API requiring user input is called). Is it possible to do something similar for promises (and simultaneously making it impossible to store the promise for later)? (perhaps by forcing the user gesture to expire one second after the promise is resolved?) bug 1392624 is also relevant (because it also involves transferring the isHandlingUserInput flag across async functions).
Flags: needinfo?(rob)
(In reply to Andrew Swan [:aswan] from comment #1) > In this particular case, you don't actually need to check for the permission > before requesting it, request() will just quietly return true if you request > a permission you already have. Not in my experience, I just tested it again. It re-requests permission for the same domain that the user has already added permissions for. In my addon I'm using promises (the code sample above uses callbacks for simplicity,) but the call to request() is the same. If you're interested, the code I'm using is here: https://gitlab.com/adimit/gitlab-time-tracking-button/blob/master/src/ChromeAdapter.js#L13 Note the workaround I'm using for FF at the moment. The permission requesting code is a bit lower: return new Promise((resolve, reject) => { this.chrome.permissions.request({ origins: [url], }, (granted) => { if (granted) { resolve(granted); } else { reject(Error(`Permission for ${url} not granted`)); } }); }); I don't think this should in principle behave any differently than a plain callback-call to request(). If you think this is a bug, I could file a new one.
status-firefox57: --- → wontfix
Priority: -- → P3
Whiteboard: [permissons]
Product: Toolkit → WebExtensions
status-firefox57: wontfix → ---
Since bug 1438465 is duplicated here for "handling user input cross callback". I would prefer comment here instead of file another bug. bug 1352598 implemented a new API for searching. Extensions may call `search.search(name, searchTerms, tabId)` to perform a searching. While, it is common to pass a newly created tab's id as `tabId` parameter. But `tabs.create` is async function, which means user input status is lost, and `search.search` won't work after it.

i can confirm aleks' comments from three years ago while working with 74.0b9:

  1. checking whether url(!) permissions are granted via chrome.permissions.contains beforehand cause the infamuous
    'permissions.request may only be called from a user input handler'

  2. calling chrome.permissions.request straight for urls works as expected... in google chrome - chrome recognizes that the permission has already been granted when declared in manifest.json#permissions. at its current stage – 74.0b9 – firefox makes me declare the same permission in optional_permissions or it won't grant it; in some situations this requires the use of <all_urls> – something we certainly try to avoid – since we cannot foresee which sites the user may want to access.

may i ask to put this on higher priority?

(In reply to tom from comment #7)

i can confirm aleks' comments from three years ago while working with 74.0b9:

  1. checking whether url(!) permissions are granted via chrome.permissions.contains beforehand cause the infamuous
    'permissions.request may only be called from a user input handler'

Not true. The contains method can unconditionally be called.

  1. calling chrome.permissions.request straight for urls works as expected... in google chrome - chrome recognizes that the permission has already been granted when declared in manifest.json#permissions. at its current stage – 74.0b9 – firefox makes me declare the same permission in optional_permissions or it won't grant it; in some situations this requires the use of <all_urls> – something we certainly try to avoid – since we cannot foresee which sites the user may want to access.

Requiring a permission to be matched by a value in optional_permissions before it can be requested as an optional permission looks like intended behavior.
The fact that an optional permission is not prompted for if there is already a matching mandatory permission is a bug that tracked at bug 1502987.

may i ask to put this on higher priority?

The current priority looks about right. There is no need to call contains() before request, and removing that call resolves the issue.
Extending the lifetime of the user interaction is somewhat risky, because it can be abused to extend the user interaction for (much) longer than desired.

(In reply to Rob Wu [:robwu] from comment #8)

  1. checking whether url(!) permissions are granted via chrome.permissions.contains beforehand cause the infamuous
    'permissions.request may only be called from a user input handler'

Not true. The contains method can unconditionally be called.

sorry, i wasn't clear enough: a subsequent call to #request causes the error above, not the call to #contains itself

  1. calling chrome.permissions.request straight for urls works as expected... in google chrome - chrome recognizes that the permission has already been granted when declared in manifest.json#permissions. at its current stage – 74.0b9 – firefox makes me declare the same permission in optional_permissions or it won't grant it; in some situations this requires the use of <all_urls> – something we certainly try to avoid – since we cannot foresee which sites the user may want to access.

Requiring a permission to be matched by a value in optional_permissions before it can be requested as an optional permission looks like intended behavior.

even if we have to maintain it in both permissions and optional_permissions in manifest.json?

The fact that an optional permission is not prompted for if there is already a matching mandatory permission is a bug that tracked at bug 1502987.

ah, thank you. so i guess that one needs higher prio ;)

Severity: normal → S4
Status: UNCONFIRMED → NEW
Ever confirmed: true

The severity field for this bug is relatively low, S4. However, the bug has 3 duplicates.
:rpl, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Flags: needinfo?(lgreco)

We discussed about this bug in a pretty recent triage and agreed on setting it as an S4, the duplicates were already known at that time, and so we will keep the S4 set for now.

(But there is still works ongoing around the changes to host permissions behaviors in manifest_version 3 and so I added it as a SeeAlso to the related tracking bug, as something to keep it in mind and consider to bump in priority).

Flags: needinfo?(lgreco)

Inability to call any async function before permission request sounds like a serious obstacle on the route to manifest v3 and service worker.

Consider an add-on that declares some optional permissions and specific permission required to complete user actions depends on configuration. Non-persistent background page or service worker may be unloaded any time. Since browser.storage.local.get() is an async method, it is impossible to conditionally request optional permission from browser.action.onClicked handler. With persistent background page it is not an issue since configuration may be fetched from storage on extension load, so it is almost certainly ready in user action handlers.

I just confirmed the issue with "manifest_version": 3 in Firefox-109. I can post a tiny demo add-on.

You need to log in before you can comment on or make changes to this bug.