1006237 - Unify datastore permission check in nsIDataStoreService.checkPermission()

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[(no longer active)]

We're going to enable access to the DataStore API based on a permission on the Gecko side which will only be granted to the Loop app for 2.0.

My suggestion for the name of the permission is "datastore-temporary-permission", feel free to suggest a better name if you can think of one.  Andrea, can you please write a patch for this?  Thanks!

Fernando, Marie, do you mind please CCing all of the interested parties on this bug?  Thanks!

Comment 1

[Andrea Marchesini [:baku]]

I think we can 'expand' what dom.testing.datastore_enabled_for_hosted_apps does. At the moment we enable the permissions for any app during the mochitest. I guess we can use this property, renaming it, for the Loop app.

Comment 2

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: nsIDataStoreService.checkPermission()

Comment 3

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: Add datastore-temporary-permission

I still need to work on the tests but in the meantime any feedback is appreciated :)

Comment 4

[Gene Lian (I already quit Mozilla)]

I'm wondering why shouldn't this depend on bug 942641? We have to provide a prompting dialogue to allow privileged apps to access the data store. Right?

Comment 5

[Andrea Marchesini [:baku]]

Comment on attachment 8418107 [details] [diff] [review]
Part 1: nsIDataStoreService.checkPermission()

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

::: dom/datastore/DataStoreChangeNotifier.jsm
@@ +73,5 @@
>  
> +    // No check has to be done when the message is 'child-process-shutdown'.
> +    if (aMessage.name != "child-process-shutdown") {
> +      let secMan = Cc["@mozilla.org/scriptsecuritymanager;1"]
> +                     .getService(Ci.nsIScriptSecurityManager);

if (!aMessage.principal) or.. better: if (!("principal" in aMessage)) return;

Comment 6

[(no longer active)]

(In reply to comment #4)
> I'm wondering why shouldn't this depend on bug 942641? We have to provide a
> prompting dialogue to allow privileged apps to access the data store. Right?

Yes, we will do that, but this patch doesn't need to wait on bug 942641, right?

Comment 7

[Maria Angeles Oteo (:oteo)]

Removing the dependency with 1.0 loop mobile client (corresponding to 2.0 FxOS version) as it was agreed with Vivien that contacts is going to check if Loop is installed via mozApps.

Comment 8

[(no longer active)]

(In reply to comment #7)
> Removing the dependency with 1.0 loop mobile client (corresponding to 2.0 FxOS
> version) as it was agreed with Vivien that contacts is going to check if Loop
> is installed via mozApps.

What about the FB contacts dependency?

Comment 9

[Maria Angeles Oteo (:oteo)]

According to https://bugzilla.mozilla.org/show_bug.cgi?id=1007933#c11 it seems there are also legal issues with that integration, that's the reason for removing the dependency for the time being.

Comment 10

[(no longer active)]

(In reply to comment #9)
> According to https://bugzilla.mozilla.org/show_bug.cgi?id=1007933#c11 it seems
> there are also legal issues with that integration, that's the reason for
> removing the dependency for the time being.

I see, thanks for the update!

Comment 11

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Add nsIDataStoreService.checkPermission()

Even if we are not adding the temporary permission for Loop, I think we can still use this clean up that moves all the permission conditions to a single place.

Comment 12

[(no longer active)]

Comment on attachment 8438653 [details] [diff] [review]
Add nsIDataStoreService.checkPermission()

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

Thanks for doing this!

::: dom/datastore/DataStoreChangeNotifier.jsm
@@ +80,5 @@
> +                     .getService(Ci.nsIScriptSecurityManager);
> +      let uri = Services.io.newURI(aMessage.principal.origin, null, null);
> +      let principal = secMan.getAppCodebasePrincipal(uri,
> +        aMessage.principal.appId, aMessage.principal.isInBrowserElement);
> +      if (!principal || !dataStoreService.checkPermission(principal)) {

This null check on the principal here is not required right?  Because CheckPermission does that already.

Comment 13

[Andrea Marchesini [:baku]]

Comment on attachment 8438653 [details] [diff] [review]
Add nsIDataStoreService.checkPermission()

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

lgtm. Can I see it on try? Thanks!

::: dom/apps/src/Webapps.jsm
@@ +619,5 @@
> +    let secMan = Cc["@mozilla.org/scriptsecuritymanager;1"]
> +                   .getService(Ci.nsIScriptSecurityManager);
> +    let principal = secMan.getAppCodebasePrincipal(uri, aId,
> +                                                   /*mozbrowser*/ false);
> +    if (!principal || !dataStoreService.checkPermission(principal)) {

no !principal is needed.

Comment 14

[Fernando Jiménez Moreno [:ferjm]]

Thanks Andrea and Ehsan!

Try build at: https://tbpl.mozilla.org/?tree=Try&rev=03b11052febf

Comment 15

[Fernando Jiménez Moreno [:ferjm]]

Try looked good to me, so https://hg.mozilla.org/integration/b2g-inbound/rev/d82b4144144e

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/d82b4144144e