1173523 - Change nsIPermission to expose a principal rather than a host, appId, and inBrowserElement

Status: RESOLVED FIXED

(Core :: Permission Manager, defect)

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Expose an nsIPrincipal from nsIPermission rather than a host string, appId, and isInBrowserElement

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Tests for new methods nsIPrincipal::MatchesURI and nsIPrincipal::Matches

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 3: Update PluginContent to use new API for nsIPermission

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 4: Update about:permissions to use new API for nsIPermission

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 5: Update the preferences permissions UI to use new API for nsIPermission

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 6: Update translation to use new API for nsIPermission

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 7: Update advanced.js to use new API for nsIPermission

Comment 8

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 8: Update ForgetAboutSite.jsm to use new API for nsIPermission

Comment 9

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 9: Update pageinfo to use the new API for nsIPermission

Comment 10

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 10: Update nsIPermission and nsIPermissionManager tests to use new API for nsIPermission

Comment 11

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 11: Update tests for cookie permissions to use new API for nsIPermission

Comment 12

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 12: Update tests for plugin permissions to use new API for nsIPermission

Comment 13

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 13: Update SpecialPowers to use new API for nsIPermission

Comment 14

[Nika Layzell [:nika] (ni? for response)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bed6a33ba703

Comment 15

[Nika Layzell [:nika] (ni? for response)]

(In reply to Michael Layzell [:mystor] from comment #14)
> try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bed6a33ba703

Oops - apparently I borked something in my recent changes - I'll have updated patches once I fix the problems on friday.

Comment 16

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 14: Update reftests due to removal of <file> magic host

Reftests got broken, as they depended on the <file> magic host to give file URIs permission to open XUL/XBL.
This patch changes reftests to instead use the preference to allow <file> URIs to open XUL/XBL.

Comment 17

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 13: Update SpecialPowers to use new API for nsIPermission

When using e10s, a fake permission was passed through IPC, which no longer matched the format of the real permission.

Comment 18

[(no longer active)]

Comment on attachment 8620538 [details] [diff] [review]
Part 1: Expose an nsIPrincipal from nsIPermission rather than a host string, appId, and isInBrowserElement

Review of attachment 8620538 [details] [diff] [review]:
-----------------------------------------------------------------

::: extensions/cookie/nsPermission.cpp
@@ +31,2 @@
>  {
> +  nsCOMPtr<nsIPrincipal> grip = mPrincipal;

Nit: please call this copy.

@@ +66,5 @@
> +nsPermission::Matches(nsIPrincipal* aPrincipal, bool aExactHost, bool* aMatches)
> +{
> +  *aMatches = false;
> +
> +  NS_ENSURE_ARG_POINTER(aPrincipal);

Please check the same for aMatches.  You should do these two in the beginning of the function.

@@ +84,5 @@
> +  rv = mPrincipal->GetOriginSuffix(ourSuffix);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  if (theirSuffix != ourSuffix) {
> +    *aMatches = false;

Nit: no need to set this to false twice.

@@ +101,5 @@
> +  // Get the hosts so we can compare them
> +  nsAutoCString theirHost;
> +  rv = theirURI->GetHost(theirHost);
> +  if (NS_FAILED(rv) || theirHost.IsEmpty()) {
> +    *aMatches = false;

Ditto.

@@ +108,5 @@
> +
> +  nsAutoCString ourHost;
> +  rv = ourURI->GetHost(ourHost);
> +  if (NS_FAILED(rv) || ourHost.IsEmpty()) {
> +    *aMatches = false;

And here.

@@ +130,5 @@
> +  // NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS.
> +  while (theirHost != ourHost) {
> +    rv = tldService->GetNextSubDomain(theirHost, theirHost);
> +    if (NS_FAILED(rv)) {
> +      *aMatches = false;

And here.

::: extensions/cookie/nsPermissionManager.cpp
@@ +1427,5 @@
>    if (entry) {
>      return entry;
>    }
>  
> +  // If aExactHostMatch wasn't true, we can check if the base domain has a permission entry.

I prefer if you did this in the other bug that is filed for this, together with the reftest permission change.

::: extensions/cookie/test/unit/test_permmanager_local_files.js
@@ -43,5 @@
> -  // Add the magic "<file>" permission and make sure all "file://" now have the permission.
> -  pm.addFromPrincipal(getPrincipalFromURIString("http://<file>"), "test/local-files", pm.ALLOW_ACTION, 0, 0);
> -  do_check_eq(pm.testPermissionFromPrincipal(principal, "test/local-files"), pm.ALLOW_ACTION);
> -  do_check_eq(pm.testPermissionFromPrincipal(witnessPrincipal, "test/local-files"), pm.ALLOW_ACTION);
> -  do_check_eq(pm.testPermissionFromPrincipal(fileInDirPrincipal, "test/local-files"), pm.ALLOW_ACTION);

This should move to the other bug as well.

::: netwerk/base/nsIPermission.idl
@@ +12,1 @@
>  [scriptable, uuid(cfb08e46-193c-4be7-a467-d7775fb2a31e)]

Please rev the uuid of nsIPermission.

Comment 19

[(no longer active)]

Comment on attachment 8620540 [details] [diff] [review]
Part 2: Tests for new methods nsIPrincipal::MatchesURI and nsIPrincipal::Matches

Review of attachment 8620540 [details] [diff] [review]:
-----------------------------------------------------------------

::: extensions/cookie/test/unit/test_permmanager_matches.js
@@ +32,5 @@
> +
> +  // Add some permissions
> +  let uri0 = NetUtil.newURI("http://google.com/", null, null);
> +  let uri1 = NetUtil.newURI("http://hangouts.google.com/", null, null);
> +  let uri2 = NetUtil.newURI("http://google.org/", null, null);

Just for extra fun, please try a URL with https, and one with a custom port.

::: extensions/cookie/test/unit/test_permmanager_matchesuri.js
@@ +13,5 @@
> +  let uri0 = NetUtil.newURI("http://google.com:9091/just/a/path", null, null);
> +  let uri1 = NetUtil.newURI("http://hangouts.google.com:9091/some/path", null, null);
> +  let uri2 = NetUtil.newURI("http://google.com:9091/", null, null);
> +  let uri3 = NetUtil.newURI("http://google.org:9091/", null, null);
> +  let uri4 = NetUtil.newURI("http://deeper.hangouts.google.com:9091/", null, null);

Please include https in the URLs to test as well.

Comment 20

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Expose an nsIPrincipal from nsIPermission rather than a host string, appId, and isInBrowserElement

Updated in response to feedback, and moved changes related to removing <file> to Bug 817007

Comment 21

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Expose an nsIPrincipal from nsIPermission rather than a host string, appId, and isInBrowserElement

Fix typo and mark Part 14 as obsolete

Comment 22

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8621685 [details] [diff] [review]
Part 13: Update SpecialPowers to use new API for nsIPermission

Review of attachment 8621685 [details] [diff] [review]:
-----------------------------------------------------------------

thanks, this is nice.

Comment 23

[Florian Quèze [:florian]]

Comment on attachment 8620545 [details] [diff] [review]
Part 6: Update translation to use new API for nsIPermission

Is this now treating http://example.com and https://example.com as 2 different websites for permission purposes? If so, is this something we want for the cases where the permission is saving the "never bother me again" information (eg. the translation case) rather than "grant permission to do something" (eg. getUserMedia)?

Comment 24

[Florian Quèze [:florian]]

Comment on attachment 8620548 [details] [diff] [review]
Part 9: Update pageinfo to use the new API for nsIPermission

Review of attachment 8620548 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/pageinfo/permissions.js
@@ +34,5 @@
>    var permTab = document.getElementById("permTab");
>    if (SitePermissions.isSupportedURI(uri)) {
>      gPermURI = uri;
>      var hostText = document.getElementById("hostText");
> +    hostText.value = gPermURI.spec;

Assuming my guess is correct about what you are trying to do, this should be gPermURI.prePath, not gPermURI.spec.

Comment 25

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 9: Update pageinfo to use the new API for nsIPermission

This is related to my efforts to change over to treating http://example.com and https://example.com as 2 different websites for permission purposes. This particular issue is focused on changing the API such that the backend change won't require breakage when the actual backend changing patch lands (which is happening in bug 1165263). 

(Also, you're right - I've changed gPermURI.spec to gPermURI.prePath)

Comment 26

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Tests for new methods nsIPrincipal::MatchesURI and nsIPrincipal::Matches

Nicer and more complete tests for MatchesURI and Matches

Comment 27

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8623315 [details] [diff] [review]
Part 1: Expose an nsIPrincipal from nsIPermission rather than a host string, appId, and isInBrowserElement

Review of attachment 8623315 [details] [diff] [review]:
-----------------------------------------------------------------

::: extensions/cookie/nsPermission.cpp
@@ +69,5 @@
> +  NS_ENSURE_ARG_POINTER(aMatches);
> +
> +  *aMatches = false;
> +
> +  // If the principals are equal, than they match.

s/than/then/

Comment 28

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8620546 [details] [diff] [review]
Part 7: Update advanced.js to use new API for nsIPermission

Review of attachment 8620546 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/preferences/in-content/advanced.js
@@ -605,5 @@
>      // remove the permission
>      var pm = Components.classes["@mozilla.org/permissionmanager;1"]
>                         .getService(Components.interfaces.nsIPermissionManager);
>      pm.removePermission(perm);
> -    pm.removePermission(perm);

This should be fixed in the patch that introduced these (bug 1170200). I left a comment in that bug with a question.

Comment 29

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 7: Update advanced.js to use new API for nsIPermission

I looked back at this file, and apparently it was late when I worked on it, because I did _everything_ wrong. Here's a new patch which should not have the problems of the old version. (hopefully)

Comment 30

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Expose an nsIPrincipal from nsIPermission rather than a host string, appId, and isInBrowserElement

Grammar nit fix

Comment 31

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 7: Update advanced.js to use new API for nsIPermission

Fixup patch such that it will apply on changes from 1170200

Comment 32

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8620541 [details] [diff] [review]
Part 3: Update PluginContent to use new API for nsIPermission

Review of attachment 8620541 [details] [diff] [review]:
-----------------------------------------------------------------

Looks mostly right - just a few loose ends to tie up.

::: browser/base/content/browser-plugins.js
@@ +293,5 @@
>        dismissed: !showNow,
>        eventCallback: this._clickToPlayNotificationEventCallback,
>        primaryPlugin: primaryPluginPermission,
>        pluginData: pluginData,
> +      principal: principal

Nit - please keep the trailing comma to reduce blame

::: browser/modules/PluginContent.jsm
@@ +770,5 @@
>  
>        let permissionObj = Services.perms.
>          getPermissionObject(principal, pluginInfo.permissionString, false);
>        if (permissionObj) {
> +        pluginInfo.pluginPermissionSpec = permissionObj.principal.originNoSuffix;

I guess we call an origin without a suffix a spec? In any case, you need to update _setupSingleState in browser/base/content/urlbarBindings.xml to use this renamed property.

While you're at it, you'll probably want to update the "__host__" in _setupSingleState to "__spec__" for consistency.

@@ +784,5 @@
>  
>      this.global.sendAsyncMessage("PluginContent:ShowClickToPlayNotification", {
>        plugins: [... this.pluginData.values()],
>        showNow: showNow,
> +      location: location

Nit - please keep the trailing comma to reduce blame

Comment 33

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 3: Update PluginContent to use new API for nsIPermission

I've fixed up the code, and decided that prePath is probably a better name for the originNoSuffix than spec.

Comment 34

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8626604 [details] [diff] [review]
Part 3: Update PluginContent to use new API for nsIPermission

Review of attachment 8626604 [details] [diff] [review]:
-----------------------------------------------------------------

Assuming a green try run for tests under browser/base/content/test/plugins, this is good by me. Thanks mystor!

::: browser/base/content/urlbarBindings.xml
@@ +2403,5 @@
>            // from the tracking protection allowlist.
> +          let normalizedUrl = Services.io.newURI(
> +            "https://" + gBrowser.selectedBrowser.currentURI.hostPort,
> +            null, null);
> +          Services.perms.remove(normalizedUrl,

Might as well put 2408 and 2407 on the same line.

Comment 35

[(no longer active)]

Comment on attachment 8620544 [details] [diff] [review]
Part 5: Update the preferences permissions UI to use new API for nsIPermission

Review of attachment 8620544 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/preferences/permissions.js
@@ +94,5 @@
> +        let uri = Services.io.newURI(input_url, null, null);
> +      } catch(ex) {
> +        uri = Services.io.newURI("http://" + input_url, null, null);
> +      }
> +      principal = gSecMan.getNoAppCodebasePrincipal(uri);

Nit: please use Services.scriptSecurityManager and remove gSecMan.

Comment 36

[Florian Quèze [:florian]]

Comment on attachment 8620545 [details] [diff] [review]
Part 6: Update translation to use new API for nsIPermission

Review of attachment 8620545 [details] [diff] [review]:
-----------------------------------------------------------------

This seems fine technically (and I'm not really worried about the translation case, as that code is currently preff'ed off), but I would like my question in comment 23 to be answered: is treating http:// and https:// separately desirable for the case where the information we save is "never bother me again"? If not, can we do something about it?

Comment 37

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 5: Update the preferences permissions UI to use new API for nsIPermission

gSecMan -> Services.scriptSecurityManager

Comment 38

[Nika Layzell [:nika] (ni? for response)]

This patch doesn't actually make any change with regard to treating http:// and https:// differently. That is done by bug 1165263. However, this is probably something worth talking about.

One possibility, if we decide that we want to treat "never bother me again" in the old way (based only on host, and not scheme/port), would be to, when looking up negative permissions, also lookup "http://HOST"'s permission (if there is not a relevant permission), and if it is a negative permission, using that permission. If we wanted it to work consistently, we'd want negative permissions to be attached to http://HOST instead. This could be done mostly internally, but does make the idea of "removing a permission" a bit more complex.

I also think that there is a valid usecase for people wanting to apply different permissions to different origins (say a always allow permission for https:// and a never bother me again for http://).

Comment 39

[(no longer active)]

I don't think we need to handle the case in comment 23 differently.  For all intents and purposes, http://foo.com is a completely different website that https://foo.com, and they are treated as such in pretty much everywhere.  In practice, most websites either don't have an https version of favor their https version over their http version, so hopefully this won't actually happen in the real world too often.

Comment 40

[:Margaret Leibovic]

Comment on attachment 8620543 [details] [diff] [review]
Part 4: Update about:permissions to use new API for nsIPermission

Review of attachment 8620543 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the slow review. It's been a long time since I've looked at this code, but I guess things haven't changed much in this file since I wrote this originally.

With this change, we'll now distinguish "site" items by origin instead of host. That seems like a UX change that we should probably talk to the desktop UX team about. However, I don't know what plans there are (if any) to expose about:permissions so broadly, so I don't think we need to block on that to land this change.

::: browser/components/preferences/aboutPermissions.js
@@ +76,5 @@
>      }
>  
>      // Try to find favicon for both URIs, but always prefer the https favicon.
> +    // XXX: While changing values to use origin, we will temporarially not fetch favicons
> +    // from secure URIs

Does this just fail for https URIs?

You should file follow-up bugs to address these "XXX" issues in the code.

@@ +219,4 @@
>    },
>  
>    get loginSavingEnabled() {
>      // Only say that login saving is blocked if it is blocked for both http and https.

This comment isn't necessary anymore.

@@ +834,5 @@
>      }
>  
>      let win = Services.wm.getMostRecentWindow("Toolkit:PasswordManager");
>      if (win) {
> +      win.setFilter(selectedOrigin);

I assume you tested that these password and cookie dialogs still open properly with this change.

Comment 41

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 4: Update about:permissions to use new API for nsIPermission

Comment fixes and minor bug fix with filtering of cookies

Comment 42

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Tests for new methods nsIPrincipal::MatchesURI and nsIPrincipal::Matches

fix a test failure on windows due to how it handles file URIs.

Comment 43

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Tests for new methods nsIPrincipal::MatchesURI and nsIPrincipal::Matches

Use a more correct method for distinguishing between OSes

Comment 44

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf498d466b85
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8e4f1c0c445
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea64d70eacfe
https://hg.mozilla.org/integration/mozilla-inbound/rev/89833c0bb0cd
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d5661eb966c
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d4f9f9e1b4e
https://hg.mozilla.org/integration/mozilla-inbound/rev/1caac4569616
https://hg.mozilla.org/integration/mozilla-inbound/rev/541b6faf7196
https://hg.mozilla.org/integration/mozilla-inbound/rev/72406261eccc
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bdb91114c7a
https://hg.mozilla.org/integration/mozilla-inbound/rev/82034a4560c5
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a0676b73f77
https://hg.mozilla.org/integration/mozilla-inbound/rev/b787b3f9aadc

Comment 45

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This (and everything else from ehsan's push) backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/de3fb216f066 for mass android reftest bustage:

https://treeherder.mozilla.org/logviewer.html#?job_id=11676048&repo=mozilla-inbound

Comment 46

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f696ab47f443
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7d557d359ae
https://hg.mozilla.org/integration/mozilla-inbound/rev/253472f66911
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d6abd7dccd4
https://hg.mozilla.org/integration/mozilla-inbound/rev/64a004bb6207
https://hg.mozilla.org/integration/mozilla-inbound/rev/07dfd0280593
https://hg.mozilla.org/integration/mozilla-inbound/rev/88daaf531d7c
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7c99752bf3d
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e34a3d0c309
https://hg.mozilla.org/integration/mozilla-inbound/rev/790dbeb3f000
https://hg.mozilla.org/integration/mozilla-inbound/rev/77d7097247b3
https://hg.mozilla.org/integration/mozilla-inbound/rev/f593f94b009f
https://hg.mozilla.org/integration/mozilla-inbound/rev/0593f67952f1

Comment 47

[Nika Layzell [:nika] (ni? for response)]

fixed patch is on inbound

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f696ab47f443
https://hg.mozilla.org/mozilla-central/rev/b7d557d359ae
https://hg.mozilla.org/mozilla-central/rev/253472f66911
https://hg.mozilla.org/mozilla-central/rev/8d6abd7dccd4
https://hg.mozilla.org/mozilla-central/rev/64a004bb6207
https://hg.mozilla.org/mozilla-central/rev/07dfd0280593
https://hg.mozilla.org/mozilla-central/rev/88daaf531d7c
https://hg.mozilla.org/mozilla-central/rev/e7c99752bf3d
https://hg.mozilla.org/mozilla-central/rev/8e34a3d0c309
https://hg.mozilla.org/mozilla-central/rev/790dbeb3f000
https://hg.mozilla.org/mozilla-central/rev/77d7097247b3
https://hg.mozilla.org/mozilla-central/rev/f593f94b009f
https://hg.mozilla.org/mozilla-central/rev/0593f67952f1

Comment 49

[Mike Kaply [:mkaply]]

None of the nsIPermission documentation has been updated with the permission manager changes:

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIPermission
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIPermission

Comment 50

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

(In reply to Mike Kaply [:mkaply] from comment #49)
> None of the nsIPermission documentation has been updated with the permission
> manager changes:
> 
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/
> Interface/nsIPermission
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/
> Interface/nsIPermission
What's the second link you wanted to reference?

Comment 51

[Mike Kaply [:mkaply]]

> What's the second link you wanted to reference?

Sorry.

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIPermissionManager