Closed Bug 1402957 Opened 2 years ago Closed 4 months ago

Remove permission manager APIs that only accept a URI

Categories

(Core :: Permission Manager, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox71 --- fixed

People

(Reporter: jdm, Assigned: pbz)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

In general, the nsIPermissionManager APIs that accept URIs instead of principals lead to problems like bug 1399590, since they break in surprising and subtle ways when invoked from contexts like <iframe mozbrowser>. If we force all consumers to pass in a principal, we increase the likelihood that we actually test permissions using the expected origin attributes.
We may also want to add new nsIPermissionManager APIs which make it easier to use them if we don't have a Principal without performing expensive nsIPrincipal creation. e.g. a (nsIURI*, const OriginAttributes&) api.

The hardest part of this change will be finding all JS consumers of the APIs and rewriting them.
(In reply to Nika Layzell [:mystor] from comment #1)
> We may also want to add new nsIPermissionManager APIs which make it easier
> to use them if we don't have a Principal without performing expensive
> nsIPrincipal creation. e.g. a (nsIURI*, const OriginAttributes&) api.
> 
> The hardest part of this change will be finding all JS consumers of the APIs
> and rewriting them.

Yeah, that sounds annoying. :)
Priority: -- → P3
Depends on: 1330467
Depends on: 1531303
Depends on: 1574105
Depends on: 1574110
Depends on: 1574124
Depends on: 1574130
Assignee: nobody → pbz
Status: NEW → ASSIGNED
Depends on: 1574469
Depends on: 1574473
Depends on: 1574477
Depends on: 1574480
Depends on: 1574483
Depends on: 1574484
Depends on: 1574486

I've filed bugs for all PermissionManager / SitePermission usages I have found so far. If you know more nsIURI based usages please file a bug or let me know!

JS consumers are especially tricky to find. For those I've used regex:

SitePermissions\s*\.(getAllByURI|get|set|remove|isSupportedURI)\s*\(
\b(Services\s*\.perms|perms|pm|ps|permManager|Ci.nsIPermissionManager)\s*\.(add|getAllForURI|remove|testPermission|testExactPermission|getPermissionObjectForURI)\s*\(
Depends on: 1575901

🚀

Pushed by pzuhlcke@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7ddb6f049e14
Refactored SitePermission tests to use principals. r=johannh
https://hg.mozilla.org/integration/autoland/rev/1cd25a1bf819
Removed deprecated SitePermission URI methods. r=johannh
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

We did not remove the methods from https://searchfox.org/mozilla-central/source/netwerk/base/nsIPermissionManager.idl, which this bug was also about, at least how I perceived it. Paul, should we reopen?

Flags: needinfo?(pbz)

Yes please. I'm currently working on the remaining methods in nsIPermissionManager.

Status: RESOLVED → REOPENED
Flags: needinfo?(pbz)
Resolution: FIXED → ---
Pushed by pzuhlcke@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ed5794453f69
Refactored nsIPermissionManager tests to use principals. r=johannh
https://hg.mozilla.org/integration/autoland/rev/781a9824fd99
Refactored remaining tests using nsIPermissionManager URI methods to principals. r=johannh
https://hg.mozilla.org/integration/autoland/rev/e3641f082470
Refactored Marionette nsIPermissionManager usages for principals. r=johannh,whimboo
https://hg.mozilla.org/integration/autoland/rev/feba8126032f
Removed URI based nsIPermissionManager methods. r=johannh,Ehsan
You need to log in before you can comment on or make changes to this bug.