Closed Bug 1173523 Opened 5 years ago Closed 4 years ago

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

Categories

(Core :: Permission Manager, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: Nika, Assigned: Nika)

References

Details

(Keywords: addon-compat, dev-doc-needed)

Attachments

(13 files, 14 obsolete files)

5.53 KB, patch
florian
: review+
Details | Diff | Splinter Review
1.29 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
17.73 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
4.68 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
1.14 KB, patch
jimm
: review+
Details | Diff | Splinter Review
2.47 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
2.61 KB, patch
florian
: review+
Details | Diff | Splinter Review
33.02 KB, patch
Details | Diff | Splinter Review
10.21 KB, patch
Details | Diff | Splinter Review
16.19 KB, patch
mconley
: review+
Details | Diff | Splinter Review
9.05 KB, patch
Details | Diff | Splinter Review
25.37 KB, patch
Details | Diff | Splinter Review
14.18 KB, patch
Details | Diff | Splinter Review
No description provided.
(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.
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.
When using e10s, a fake permission was passed through IPC, which no longer matched the format of the real permission.
Attachment #8620552 - Attachment is obsolete: true
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.
Attachment #8620538 - Flags: review?(ehsan) → review+
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.
Attachment #8620540 - Flags: review?(ehsan) → review+
Attachment #8620541 - Flags: review?(mconley)
Attachment #8620543 - Flags: review?(margaret.leibovic)
Attachment #8620544 - Flags: review?(ehsan)
Attachment #8620545 - Flags: review?(florian)
Attachment #8620546 - Flags: review?(benjamin)
Attachment #8621685 - Flags: review?(jmaher)
Attachment #8620551 - Flags: review?(jmathies)
Attachment #8620547 - Flags: review?(ehsan)
Attachment #8620548 - Flags: review?(ehsan)
Attachment #8620549 - Flags: review?(ehsan)
Attachment #8620550 - Flags: review?(ehsan)
Blocks: 817007
Updated in response to feedback, and moved changes related to removing <file> to Bug 817007
Attachment #8620538 - Attachment is obsolete: true
Fix typo and mark Part 14 as obsolete
Attachment #8621684 - Attachment is obsolete: true
Attachment #8623264 - Attachment is obsolete: true
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.
Attachment #8621685 - Flags: review?(jmaher) → review+
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 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.
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)
Attachment #8620548 - Attachment is obsolete: true
Attachment #8620548 - Flags: review?(ehsan)
Attachment #8623664 - Flags: review?(florian)
Attachment #8623664 - Flags: review?(florian) → review+
Nicer and more complete tests for MatchesURI and Matches
Attachment #8620540 - Attachment is obsolete: true
Attachment #8620551 - Flags: review?(jmathies) → review+
Attachment #8620546 - Flags: review?(benjamin) → review?(jaws)
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 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.
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)
Attachment #8620546 - Attachment is obsolete: true
Attachment #8620546 - Flags: review?(jaws)
Attachment #8624719 - Flags: review?(jaws)
Fixup patch such that it will apply on changes from 1170200
Attachment #8624719 - Attachment is obsolete: true
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
Attachment #8620541 - Flags: review?(mconley) → review-
I've fixed up the code, and decided that prePath is probably a better name for the originNoSuffix than spec.
Attachment #8620541 - Attachment is obsolete: true
Attachment #8626604 - Flags: review?(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.
Attachment #8626604 - Flags: review?(mconley) → review+
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.
Attachment #8620544 - Flags: review?(ehsan) → review+
Attachment #8620547 - Flags: review?(ehsan) → review+
Attachment #8620549 - Flags: review?(ehsan) → review+
Attachment #8620550 - Flags: review?(ehsan) → review+
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?
Attachment #8620545 - Flags: review?(florian) → review+
gSecMan -> Services.scriptSecurityManager
Attachment #8620544 - Attachment is obsolete: true
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://).
Flags: needinfo?(ehsan)
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.
Flags: needinfo?(ehsan)
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.
Attachment #8620543 - Flags: review?(margaret.leibovic) → review+
Comment fixes and minor bug fix with filtering of cookies
Attachment #8620543 - Attachment is obsolete: true
Keywords: addon-compat
fix a test failure on windows due to how it handles file URIs.
Attachment #8623751 - Attachment is obsolete: true
Use a more correct method for distinguishing between OSes
Attachment #8630127 - Attachment is obsolete: true
Blocks: 1180871
fixed patch is on inbound
Flags: needinfo?(michael)
Depends on: 1185328
Depends on: 1185343
Depends on: 1185365
Depends on: 1186034
Depends on: 1189073
Depends on: 1190175
Depends on: 1193235
No longer depends on: 1193235
See Also: → 1188348
Depends on: 1220350
No longer depends on: 1220350
(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?
Depends on: 1280775
Depends on: 1228219
You need to log in before you can comment on or make changes to this bug.