Closed
Bug 1394553
Opened 7 years ago
Closed 7 years ago
Add prompt for manifest:devtools_page permission
Categories
(WebExtensions :: General, enhancement, P1)
WebExtensions
General
Tracking
(firefox57 fixed)
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: zombie, Assigned: zombie)
References
Details
Attachments
(2 files, 1 obsolete file)
+++ This bug was initially created as a clone of Bug #1394134 +++ This will need some new code, so split from bug 1394134 as I'd like to keep that to permission strings only.
Updated•7 years ago
|
Assignee: nobody → lgreco
Assignee | ||
Updated•7 years ago
|
Assignee: lgreco → tomica
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8904336 -
Flags: review?(aswan)
Attachment #8904337 -
Flags: review?(aswan)
Attachment #8904338 -
Flags: review?(aswan)
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8904336 [details] Bug 1394553 - Add support for `manifest:keys` used as permissions https://reviewboard.mozilla.org/r/176122/#review182534 Sorry for the slow reply but I'm still grappling with all the different ways we use permisisons and this still isn't sitting right for me. I think we're in agreement that: - Any code that wants to make a decision about whether to allow some action should be using `extension.hasPermission()` (note that there are separate implementations of this method in the parent process and in content processes) - Everything the front-end needs to know about is accessed with `extension.userPermissions`. You identified a problem with `hasPermission()` and manifest normalization, and I agree that should be fixed. But in the short term, I'm more concerned about how we populate `userPermissions`. With this patch, it gets a `manifest:` entry for every present manifest entry that is also referenced from a webextensions schema. That both causes unnecessary items to be put into userPermissions, but it is also roundabout when we want to add a prompt for some other manifest key. It also wouldn't really work for nested manifest keys. For those reasons, I would really prefer explicit logic in the `userPermissions` getter to add specific manifest keys...
Assignee | ||
Comment 8•7 years ago
|
||
(In reply to Andrew Swan [:aswan] from comment #7) > Sorry for the slow reply but I'm still grappling with all the different ways > we use permisisons and this still isn't sitting right for me. I think we're > in agreement that: > - Any code that wants to make a decision about whether to allow some action > should be using `extension.hasPermission()` (note that there are separate > implementations of this method in the parent process and in content > processes) > - Everything the front-end needs to know about is accessed with > `extension.userPermissions`. > You identified a problem with `hasPermission()` and manifest normalization, > and I agree that should be fixed. I also added arguments on why we should treat manifest:permissions the same as other explicit permissions in virtually every place in our code (*1), as that narrows down and simplifies our permission logic along with fixing the normalization bug. 1) ignoring browser.permissions.getAll() for a moment, as it's a special case with either solution. > But in the short term, I'm more concerned about how we populate `userPermissions`. > With this patch, it gets a `manifest:` entry for every present manifest entry that > is also referenced from a webextensions schema. > That both causes unnecessary items to be put into userPermissions, Why is that a problem, and "unnecessary" in what way? (We already add all of the `permissions` array, even those that frontend doesn't need to know about) > but it is also roundabout when we want to add a prompt for some other manifest key. It's not roundabout, it's precisely what we've been doing (pave the cowpaths) for other manifest keys used as permissions [2]. devtools_page was an exception, and only missing because we didn't bother to add it, since the `devtools` api is available in pages that exist only the manifest key is present. (And regardless of the accepted solution, we should be explicit about permissions in the schema, and add that). 2) http://searchfox.org/mozilla-central/search?q=%22manifest%3A > It also wouldn't really work for nested manifest keys. If we started to use nested manifest keys for permissions, hesPermission() also wouldn't work, so we would need to update the logic with either solution (again). > For those reasons, I would really prefer explicit logic in the `userPermissions` getter to add specific manifest keys... Explicit logic per permission is error prone, hart do test for automatically, and could lead to another proxy-like situation. I'll stop arguing in order to land a fix for this bug, but I'll be sad that we won't have test coverage from part 3 for potential future issues.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8904336 -
Attachment is obsolete: true
Attachment #8904336 -
Flags: review?(aswan)
Assignee | ||
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8904337 [details] Bug 1394553 - Part 1: Implement "devtools" permission https://reviewboard.mozilla.org/r/176124/#review183064 ::: toolkit/components/extensions/Extension.jsm:552 (Diff revision 3) > + if (this.manifest.devtools_page) { > + permissions.add("devtools"); This is what Chrome does, confirmed via `chrome.optionalPermissions.getAll()`.
Comment 12•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8904337 [details] Bug 1394553 - Part 1: Implement "devtools" permission https://reviewboard.mozilla.org/r/176124/#review183064 > This is what Chrome does, confirmed via `chrome.optionalPermissions.getAll()`. I don't get this, `chrome.optionalPermissions` is not a (documented) thing. Does this mean that if an extension includes a devtools_page, that `chrome.permissions.getAll()` includes `"devtools"` in the result?
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8904337 [details] Bug 1394553 - Part 1: Implement "devtools" permission https://reviewboard.mozilla.org/r/176124/#review183464 ::: browser/components/extensions/schemas/devtools.json:27 (Diff revision 3) > } > ] > }, > { > "namespace": "devtools", > + "permissions": ["devtools"], I don't object to this, but is also isn't necessary, the only contexts where we instantiate these APIs are devtools contexts. ::: toolkit/components/extensions/Extension.jsm:552 (Diff revision 3) > + if (this.manifest.devtools_page) { > + permissions.add("devtools"); > + } I had though we would just return "devtools" from `userPermissions` if the page is present in the manifest but this is effectively the same so *shrug*.
Attachment #8904337 -
Flags: review?(aswan) → review+
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8904338 [details] Bug 1394553 - Part 2: Prune the GRANTED_WITHOUT_USER_PROMPT list https://reviewboard.mozilla.org/r/176126/#review183472
Attachment #8904338 -
Flags: review?(aswan) → review+
Assignee | ||
Comment 15•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8904337 [details] Bug 1394553 - Part 1: Implement "devtools" permission https://reviewboard.mozilla.org/r/176124/#review183464 > I don't object to this, but is also isn't necessary, the only contexts where we instantiate these APIs are devtools contexts. That's implicit (and could change), I think it's better to be explicit about permissions. > I had though we would just return "devtools" from `userPermissions` if the page is present in the manifest but this is effectively the same so *shrug*. This is required by the schema change above (and it's less of a special case here than in `userPermissions`).
Comment 16•7 years ago
|
||
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 69d084bc44b5 -d 867e6e5787fa: rebasing 419254:69d084bc44b5 "Bug 1394553 - Part 1: Implement "devtools" permission r=aswan" merging browser/locales/en-US/chrome/browser/browser.properties merging toolkit/components/extensions/Extension.jsm warning: conflicts while merging toolkit/components/extensions/Extension.jsm! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment 17•7 years ago
|
||
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 69d084bc44b5 -d 650520572447: rebasing 419256:69d084bc44b5 "Bug 1394553 - Part 1: Implement "devtools" permission r=aswan" merging browser/locales/en-US/chrome/browser/browser.properties merging toolkit/components/extensions/Extension.jsm warning: conflicts while merging toolkit/components/extensions/Extension.jsm! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 20•7 years ago
|
||
Pushed by tomica@gmail.com: https://hg.mozilla.org/integration/autoland/rev/2b51c082d3f4 Part 1: Implement "devtools" permission r=aswan https://hg.mozilla.org/integration/autoland/rev/9c580f81df29 Part 2: Prune the GRANTED_WITHOUT_USER_PROMPT list r=aswan
Comment 21•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2b51c082d3f4 https://hg.mozilla.org/mozilla-central/rev/9c580f81df29
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 22•6 years ago
|
||
Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.
Flags: needinfo?(tomica)
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•