Closed Bug 775199 Opened 9 years ago Closed 9 years ago

Contacts API: remove whitelist approach for permissions.

Categories

(Core :: DOM: Device Interfaces, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17
blocking-basecamp +

People

(Reporter: gwagner, Assigned: gwagner)

References

Details

Attachments

(2 files)

No description provided.
Attached patch patchSplinter Review
Comment on attachment 643481 [details] [diff] [review]
patch

The // XXX REMOVEME part should be removed once all related gaia changes landed as well.
Attachment #643481 - Flags: review?(mounir)
blocking-basecamp: --- → ?
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 on attachment 643481 [details] [diff] [review]
patch

Actually meant r=me... see the previous comment.
Attachment #643481 - Flags: review?(mounir) → review+
(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.
Just make sure to land the prepopulation part first and land that patch after *without* the "XXX REMOVEME" part.
blocking-basecamp: ? → +
Attached patch patchSplinter Review
Address review comments.
https://hg.mozilla.org/mozilla-central/rev/52a37789b6c4
https://hg.mozilla.org/mozilla-central/rev/a19d59d6e3a6
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.