Last Comment Bug 775199 - Contacts API: remove whitelist approach for permissions.
: Contacts API: remove whitelist approach for permissions.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla17
Assigned To: Gregor Wagner [:gwagner]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: 774716 776671
  Show dependency treegraph
 
Reported: 2012-07-18 11:32 PDT by Gregor Wagner [:gwagner]
Modified: 2012-08-01 19:39 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
patch (4.40 KB, patch)
2012-07-18 11:34 PDT, Gregor Wagner [:gwagner]
mounir: review+
Details | Diff | Splinter Review
patch (4.37 KB, patch)
2012-07-31 16:21 PDT, Gregor Wagner [:gwagner]
no flags Details | Diff | Splinter Review

Description Gregor Wagner [:gwagner] 2012-07-18 11:32:55 PDT

    
Comment 1 Gregor Wagner [:gwagner] 2012-07-18 11:34:36 PDT
Created attachment 643481 [details] [diff] [review]
patch
Comment 2 Gregor Wagner [:gwagner] 2012-07-18 11:36:10 PDT
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) 2012-07-18 23:31:13 PDT
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) 2012-07-18 23:31:49 PDT
Comment on attachment 643481 [details] [diff] [review]
patch

Actually meant r=me... see the previous comment.
Comment 5 Gregor Wagner [:gwagner] 2012-07-19 14:04:12 PDT
(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) 2012-07-19 15:05:08 PDT
Just make sure to land the prepopulation part first and land that patch after *without* the "XXX REMOVEME" part.
Comment 7 Gregor Wagner [:gwagner] 2012-07-31 16:21:58 PDT
Created attachment 647753 [details] [diff] [review]
patch

Address review comments.
Comment 9 Gregor Wagner [:gwagner] 2012-08-01 13:52:45 PDT
And the review comment
https://hg.mozilla.org/integration/mozilla-inbound/rev/a19d59d6e3a6

Note You need to log in before you can comment on or make changes to this bug.