Contacts API: remove whitelist approach for permissions.

RESOLVED FIXED in mozilla17

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gwagner, Assigned: gwagner)

Tracking

(Blocks: 1 bug)

unspecified
mozilla17
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+)

Details

Attachments

(2 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 643481 [details] [diff] [review]
patch
(Assignee)

Comment 2

5 years ago
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)
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 5

5 years ago
(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: ? → +
Blocks: 776671
(Assignee)

Comment 7

5 years ago
Created attachment 647753 [details] [diff] [review]
patch

Address review comments.
(Assignee)

Comment 8

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/52a37789b6c4
(Assignee)

Comment 9

5 years ago
And the review comment
https://hg.mozilla.org/integration/mozilla-inbound/rev/a19d59d6e3a6
https://hg.mozilla.org/mozilla-central/rev/52a37789b6c4
https://hg.mozilla.org/mozilla-central/rev/a19d59d6e3a6
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.