Closed Bug 1394553 Opened 2 years ago Closed 2 years ago

Add prompt for manifest:devtools_page permission

Categories

(WebExtensions :: General, enhancement, P1)

enhancement

Tracking

(firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: Tomislav, Assigned: Tomislav)

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.
Assignee: nobody → lgreco
Assignee: lgreco → tomica
Status: NEW → ASSIGNED
Attachment #8904336 - Flags: review?(aswan)
Attachment #8904337 - Flags: review?(aswan)
Attachment #8904338 - Flags: review?(aswan)
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...
(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.
Attachment #8904336 - Attachment is obsolete: true
Attachment #8904336 - Flags: review?(aswan)
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 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 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 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+
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`).
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)
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)
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
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)
Not required, thanks.
Flags: needinfo?(tomica) → qe-verify-
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.