1196665 - Add originAttributes into SpecialPowers

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Runtime, defect)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Right now SpecialPowers will pass appId and isInBrowserElement.
We should merge it into OriginAttributes.

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: WIP Patch.

WIP patch, still have some orange in try.

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: WIP Patch.

fixed more tests.

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch.

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

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch.

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 8675626 [details] [diff] [review]
Patch.

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

::: testing/specialpowers/content/SpecialPowersObserverAPI.js
@@ +322,5 @@
>        case "SPPermissionManager": {
>          let msg = aMessage.json;
>  
>          let secMan = Services.scriptSecurityManager;
> +        let principal = secMan.createCodebasePrincipal(this._getURI(msg.url), msg.originAttributes);

The message manager knows how to serialize principals. So why don't we create the principal in the content process (in specialpowersAPI), and send bonafide principal with SPPermissionManager messages? I think that would reduce the complexity of the code in specialPowersAPI.

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Thanks, that's a great idea. I should have thought of this. :P

I'll also check if it's reasonable to change the SpecialPowers API to take principal as one of the arguments, for example 
* pushPermissions
* testPermission
* hasPermission
* removePermission
* addPermission
They have an 'arg', which could be a string, or a plain JS object, or an instance of document,
it looks we could replace arg with principal, so the callers could pass document.nodePrincipal, or app.principal, or construct the principal by secMan.createCodebasePrincipal, but if the change is too large, I'll file another bug to change those API entirely.

Comment 7

[Bobby Holley (:bholley)]

(In reply to Yoshi Huang[:allstars.chh] from comment #6)
> Thanks, that's a great idea. I should have thought of this. :P
> 
> I'll also check if it's reasonable to change the SpecialPowers API to take
> principal as one of the arguments, for example 
> * pushPermissions
> * testPermission
> * hasPermission
> * removePermission
> * addPermission
> They have an 'arg', which could be a string, or a plain JS object, or an
> instance of document,
> it looks we could replace arg with principal, so the callers could pass
> document.nodePrincipal, or app.principal, or construct the principal by
> secMan.createCodebasePrincipal, but if the change is too large, I'll file
> another bug to change those API entirely.

In general I think this would be great. However, it's worth noting that untrusted content cannot access nsIPrincipal objects without SpecialPowers.wrap. So I think we can support a SpecialPowers.wrap-ed instance of nsIPrincipal, but should also support passing in a document, as well as a manual { url : foo, originAttributes : { ...} } object.

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: WIP Patch v2.

This version passes principal to SpecialPowersObserverAPI.js.
I haven't done the passing wrapped version of nsIPrincipal yet.

However I am still trying to fix an error when running dom/permission/tests/test_alarms.html.

In [1], it will pass SpecialPowers.wrap(iframe).contentDocument
but the nodePrincipal of this object seems has some problems 

From the mochitest

[21685] WARNING: 'aRv.Failed()', file /home/allstars/ssd/gecko-dev/dom/ipc/StructuredCloneData.cpp, line 72
JavaScript error: chrome://specialpowers/content/SpecialPowersObserverAPI.js, line 334: NS_ERROR_FAILURE: Failure arg 0 [nsIPermissionManager.testPermissionFromPrincipal]

will spend more time on checking this.

[1] : https://dxr.mozilla.org/mozilla-central/source/dom/permission/tests/file_framework.js#96

Comment 9

[Bobby Holley (:bholley)]

You need to do unwrapIfWrapped on the wrapped nodePrincipal.

Comment 10

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Bobby Holley (:bholley) from comment #7)
> In general I think this would be great. However, it's worth noting that
> untrusted content cannot access nsIPrincipal objects without
> SpecialPowers.wrap. So I think we can support a SpecialPowers.wrap-ed
> instance of nsIPrincipal, but should also support passing in a document, as
> well as a manual { url : foo, originAttributes : { ...} } object.

Thanks for the comments,

If we still need to support passing a 'document' then I think it's easier than passing document.nodePrincipal.

For the app cases, I found we only support principal in mozIApplication but not DOMApplication, it looks providing manifestURL is also easier.

So I didn't add another support for passing nsIPrincipal in the end.

(In reply to Bobby Holley (:bholley) from comment #9)
> You need to do unwrapIfWrapped on the wrapped nodePrincipal.

Yeah, it's working, thanks again :D

Comment 11

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 1: Add originAttributes into SpecialPowers.

Comment 12

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: update QuotaManager API fors SpecialPowers.

Comment 13

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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

Comment 14

[Bobby Holley (:bholley)]

Comment on attachment 8677392 [details] [diff] [review]
Part 1: Add originAttributes into SpecialPowers.

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

r=me with those comments addressed.

::: dom/browser-element/mochitest/browserElement_DisallowEmbedAppsInOOP.js
@@ +28,5 @@
>  
>    document.body.appendChild(iframe);
>  
> +  var context = {'url': 'http://example.org',
> +                 'originAttributes': {'inBrowser': true}};

You don't need to put quotation marks around the key name in a JS object literal. 

This occurs throughout the patch - please fix it everywhere.

::: dom/cache/test/mochitest/test_cache_orphaned_body.html
@@ +46,2 @@
>      }
> +    SpecialPowers.getStorageUsageForURI(document.documentURI, resolve, attrs);

It looks like all of this code is just passing the nodePrincipal of |document|. Can you just do that? Same for all the other test_cache* things, and various other places in this patch. Please fix them.

::: testing/mochitest/tests/Harness_sanity/test_SpecialPowersPushAppPermissions.html
@@ +95,5 @@
>    var appId = gAppsService.getAppLocalIdByManifestURL(gApp.manifestURL);
> +  is(appId, 0, "appId should become NO_APP_ID");
> +  // since gApp is uninstalled, calling SpecialPowers.hasPermission with the
> +  // app's properties (manifestURL, origin, principal, ... etc) will throw.
> +  // So we don't need to test hasPermission for the app.

Wait, why did this change with this patch? hasPermission was working before, right?

::: testing/specialpowers/components/SpecialPowersObserver.js
@@ +235,5 @@
>            // so we fake these properties.
>            msg.permission = {
> +            principal: {
> +              appId: permission.principal.appId,
> +              originAttributes: {appId: permission.principal.appId}

Shouldn't the appId item be removed, since it's contained within the originAttributes?

::: testing/specialpowers/content/specialpowersAPI.js
@@ +1852,3 @@
>      }
>  
> +    return [ principal, isSystem ];

Given that we're returning a real principal, I don't think it's necessary to return isSystem anymore, since the caller can just ask that question directly.

I filed bug 1218039 to add an ".isSystemPrincipal" to nsIPrincipal.

Comment 15

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Bobby Holley (:bholley) from comment #14)
> :::
> testing/mochitest/tests/Harness_sanity/test_SpecialPowersPushAppPermissions.
> html
> @@ +95,5 @@
> >    var appId = gAppsService.getAppLocalIdByManifestURL(gApp.manifestURL);
> > +  is(appId, 0, "appId should become NO_APP_ID");
> > +  // since gApp is uninstalled, calling SpecialPowers.hasPermission with the
> > +  // app's properties (manifestURL, origin, principal, ... etc) will throw.
> > +  // So we don't need to test hasPermission for the app.
> 
> Wait, why did this change with this patch? hasPermission was working before,
> right?
> 
There are two problems in the original test case.
1. gApp.origin is 'undefined', which is type of string.

2. The original test case has thrown in the parent side
JavaScript error: chrome://specialpowers/content/SpecialPowersObserverAPI.js, line 181: NS_ERROR_MALFORMED_URI: Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIIOService.newURI]

so inside hasPermisison in specialpowersAPI.js, the line
this._sendSyncMessage('SPPermissionManager', msg)[0] actually returns undefined, instead of false.

Back to the test case 
ok(!SpecialPowers.hasPermission('pAppPermission', context), 'pAppPermission should not have permission');
it will evaluate as true, and the test passes.

So I remove the check in my patch.

Comment 16

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8677393 [details] [diff] [review]
Part 2: update QuotaManager API fors SpecialPowers.

will merge this into Part 1

Comment 17

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Add originAttributes into SpecialPowers. v2.

addressed bobby's comment and rebase on Bug 1218039

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

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/e54c3b878993

Comment 19

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

oh, Bug 1196654 landed this morning, but I didn't notice that.
will submit a Part 2 patch to rebase on Bug 1196654.

Comment 20

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: fix browserElement_Proxy.js

Comment 21

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

back it out:
https://hg.mozilla.org/integration/b2g-inbound/rev/afa51178a780

Comment 22

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Add originAttributes into SpecialPowers. v3

rebase

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/0bf567d77e0c

Comment 24

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

https://hg.mozilla.org/mozilla-central/rev/0bf567d77e0c