775199 - Contacts API: remove whitelist approach for permissions.

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Comment 1

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 2

[Gregor Wagner [:gwagner]]

Comment on attachment 643481 [details] [diff] [review]
patch

The // XXX REMOVEME part should be removed once all related gaia changes landed as well.

Comment 3

[Mounir Lamouri (:mounir)]

Comment on attachment 643481 [details] [diff] [review]
patch

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

So, with that patch alone, no app will have contacts permissions, right? We will need to have them added at install time/generated in the profile. Is that correct?

r=me in that case and with the following comments applied:

::: dom/contacts/ContactManager.js
@@ +457,5 @@
>      let secMan = Cc["@mozilla.org/scriptsecuritymanager;1"].getService(Ci.nsIScriptSecurityManager);
>  
>      let perm = principal == secMan.getSystemPrincipal()
>                   ? Ci.nsIPermissionManager.ALLOW_ACTION
> +                 : Services.perms.testExactPermissionFromPrincipal(principal, "contacts");

Actually, you can change all that too:
let perm = Services.perms.testExactPermissionFromPrincipal(aWindow.document.nodePrincipal, "contacts");

we are now always returning ALLOW_ACTION for the system principal.

@@ +465,5 @@
> +    debug("Contacts permission: " + this.hasPrivileges);
> +
> +    // XXX REMOVEME
> +    if(!this.hasPrivileges) {
> +      dump("WARNING: contacts permssion missing! Setting to true!");

Remove that. As Vivien said, we should just push data changes when Gecko changes land.

Comment 4

[Mounir Lamouri (:mounir)]

Comment on attachment 643481 [details] [diff] [review]
patch

Actually meant r=me... see the previous comment.

Comment 5

[Gregor Wagner [:gwagner]]

(In reply to Mounir Lamouri (:mounir) from comment #3)
> Comment on attachment 643481 [details] [diff] [review]
> patch
> 
> Review of attachment 643481 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So, with that patch alone, no app will have contacts permissions, right? We
> will need to have them added at install time/generated in the profile. Is
> that correct?

So with this patch we still grant permission in the XXX Remove part but without the removeme part we rely on the pre-population.

Comment 6

[Mounir Lamouri (:mounir)]

Just make sure to land the prepopulation part first and land that patch after *without* the "XXX REMOVEME" part.

Comment 7

[Gregor Wagner [:gwagner]]

Attachment: patch

Address review comments.

Comment 8

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/52a37789b6c4

Comment 9

[Gregor Wagner [:gwagner]]

And the review comment
https://hg.mozilla.org/integration/mozilla-inbound/rev/a19d59d6e3a6

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/52a37789b6c4
https://hg.mozilla.org/mozilla-central/rev/a19d59d6e3a6