Closed Bug 1006237 Opened 6 years ago Closed 6 years ago

Unify datastore permission check in nsIDataStoreService.checkPermission()

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: ehsan, Assigned: ferjm)

Details

Attachments

(1 file, 2 obsolete files)

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!
Flags: needinfo?(amarchesini)
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.
Flags: needinfo?(amarchesini)
Assignee: nobody → ferjmoreno
Attachment #8418107 - Flags: feedback?(amarchesini)
I still need to work on the tests but in the meantime any feedback is appreciated :)
Attachment #8418110 - Flags: feedback?(amarchesini)
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 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;
Attachment #8418107 - Flags: feedback?(amarchesini) → feedback+
Attachment #8418110 - Flags: feedback?(amarchesini) → feedback+
feature-b2g: --- → 2.0
(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?
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.
No longer blocks: loop_security_change
feature-b2g: 2.0 → ---
(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?
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.
(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!
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.
Attachment #8418107 - Attachment is obsolete: true
Attachment #8418110 - Attachment is obsolete: true
Attachment #8438653 - Flags: review?(ehsan)
Attachment #8438653 - Flags: review?(amarchesini)
Summary: Enable access to the DataStore API for privileged apps for the Loop use case → Unify datastore permission check in nsIDataStoreService.checkPermission()
No longer blocks: 1002336
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.
Attachment #8438653 - Flags: review?(ehsan) → review+
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.
Attachment #8438653 - Flags: review?(amarchesini) → review+
Thanks Andrea and Ehsan!

Try build at: https://tbpl.mozilla.org/?tree=Try&rev=03b11052febf
https://hg.mozilla.org/mozilla-central/rev/d82b4144144e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.