Closed Bug 1043562 Opened 10 years ago Closed 8 years ago

Remove the dom.mozContacts.enabled pref and hide the MozContact API from code with insufficient privileges

Categories

(Core Graveyard :: DOM: Contacts, defect)

x86
macOS
defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

(Keywords: dev-doc-complete)

Attachments

(3 files, 1 obsolete file)

We don't seem to have any code that checks it.
So this was a mistake, during the WebIDL transition, but by now we've shipped a dozen Geckos with mozContacts exposed everywhere. I think we should just remove it and (in a different bug) add [AvailableIn="PrivilegedApps"] to the interface.
(In reply to Reuben Morais [:reuben] from comment #1)
> So this was a mistake, during the WebIDL transition, but by now we've
> shipped a dozen Geckos with mozContacts exposed everywhere.

Crap!  :(  I'll take this.
Assignee: nobody → ehsan
Depends on: 1045300
Depends on: 1045875
Continuing to debug the test failures I am getting on try, I need this script <https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/gaia_test.py#L144> to be loaded either in a window with chrome privileges or on a window with a few permissions set up before the window is loaded.  I don't know which window Marionette uses to run this JS script in, Malini do you have any recommendations?

Thanks!
Flags: needinfo?(mdas)
(Note that I already tried surrounding this import_script call with |self.marionette.set_context(self.marionette.CONTEXT_CHROME)| and |self.marionette.set_context(self.marionette.CONTEXT_CONTENT)| as the code in set_time does, for example, but with that, I get:

Traceback (most recent call last):
  File "/Library/Python/2.7/site-packages/marionette_client-0.7.11-py2.7.egg/marionette/marionette_test.py", line 152, in run
    self.setUp()
  File "/Users/ehsan/moz/gaia/tests/python/gaia-ui-tests/gaiatest/tests/functional/contacts/test_sort_contacts.py", line 16, in setUp
    GaiaTestCase.setUp(self)
  File "/Users/ehsan/moz/gaia/tests/python/gaia-ui-tests/gaiatest/gaia_test.py", line 751, in setUp
    self.cleanup_gaia(full_reset=False)
  File "/Users/ehsan/moz/gaia/tests/python/gaia-ui-tests/gaiatest/gaia_test.py", line 794, in cleanup_gaia
    if self.data_layer.get_setting('lockscreen.enabled'):
  File "/Users/ehsan/moz/gaia/tests/python/gaia-ui-tests/gaiatest/gaia_test.py", line 180, in get_setting
    return self.marionette.execute_async_script('return GaiaDataLayer.getSetting("%s")' % name, special_powers=True)
  File "/Library/Python/2.7/site-packages/marionette_client-0.7.11-py2.7.egg/marionette/marionette.py", line 1166, in execute_async_script
    filename=os.path.basename(frame[0]))
  File "/Library/Python/2.7/site-packages/marionette_client-0.7.11-py2.7.egg/marionette/decorators.py", line 35, in _
    return func(*args, **kwargs)
  File "/Library/Python/2.7/site-packages/marionette_client-0.7.11-py2.7.egg/marionette/marionette.py", line 613, in _send_message
    self._handle_error(response)
  File "/Library/Python/2.7/site-packages/marionette_client-0.7.11-py2.7.egg/marionette/marionette.py", line 661, in _handle_error
    raise errors.JavascriptException(message=message, status=status, stacktrace=stacktrace)
JavascriptException: JavascriptException: ReferenceError: GaiaDataLayer is not defined
TEST-UNEXPECTED-FAIL | test_sort_contacts.py test_sort_contacts.TestContacts.test_sort_contacts | JavascriptException: JavascriptException: ReferenceError: GaiaDataLayer is not defined
	execute_async_script @gaia_test.py, line 180
	inline javascript, line 483
	src: "__marionetteParams.push(marionetteScriptFinished);let __marionetteFunc = function() { return GaiaDataLayer.getSetting("lockscreen.enabled")};__marionetteFunc.apply(null, __marionetteParams); "


I _think_ that is because the code that tries to use the GaiaDataLayer object defined in gaia_data_layer.js runs with content context, which makes it not see the GaiaDataLayer object.  If we don't have a good way to work around that, what I should really do is to set up the permissions I need on the window before it loads, but I was unable to track down where the window is loaded...
Hrm, using some terrible hacks, I managed to read window.location.href from marionette, and it's app://system.gaiamobile.org/index.html.  Does this mean that this code runs in the system app?

If so, Vivien, can we modify the manifest of the system app to grant it the contacts-read permission so that it can see the mozContact API with my patches?
Flags: needinfo?(21)
Summary: Remove the dom.mozContacts.enabled pref → Remove the dom.mozContacts.enabled pref and hide the MozContact API from code with insufficient privileges
Attachment #8464412 - Attachment description: Hide the Contacts API from the contexts that lack sufficient privileges, such as Firefox desktop and Android → Hide the Contacts API from the contexts that lack sufficient privileges, such as Firefox desktop and Android (WIP)
Ehsan, I'm not sure of what you're doing here, but please remember we need a way to enable the mozContact API in Firefox Nightly, to be able to run gaia inside Firefox. _Real_ correctness about checking with app can access it is not necessary in this configuration.
(In reply to Julien Wajsberg [:julienw] from comment #7)
> Ehsan, I'm not sure of what you're doing here

I am hiding the API from any page that doesn't have either one of the contacts-read, contacts-write or contacts-create permissions.

> but please remember we need a
> way to enable the mozContact API in Firefox Nightly, to be able to run gaia
> inside Firefox. _Real_ correctness about checking with app can access it is
> not necessary in this configuration.

Can you grant these permissions to whatever URLs you need in desktop?  If yes, then this should not break your work (except that you'd need to grant the permission, of course.)
Flags: needinfo?(felash)
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #8)
> (In reply to Julien Wajsberg [:julienw] from comment #7)
> > Ehsan, I'm not sure of what you're doing here
> 
> I am hiding the API from any page that doesn't have either one of the
> contacts-read, contacts-write or contacts-create permissions.
> 
> > but please remember we need a
> > way to enable the mozContact API in Firefox Nightly, to be able to run gaia
> > inside Firefox. _Real_ correctness about checking with app can access it is
> > not necessary in this configuration.
> 
> Can you grant these permissions to whatever URLs you need in desktop?  If
> yes, then this should not break your work (except that you'd need to grant
> the permission, of course.)

I don't really know how to grant a permission ;)

We used to do this by flipping a pref, that's all I know...
Flags: needinfo?(felash)
(In reply to comment #9)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment
> #8)
> > (In reply to Julien Wajsberg [:julienw] from comment #7)
> > > Ehsan, I'm not sure of what you're doing here
> > 
> > I am hiding the API from any page that doesn't have either one of the
> > contacts-read, contacts-write or contacts-create permissions.
> > 
> > > but please remember we need a
> > > way to enable the mozContact API in Firefox Nightly, to be able to run gaia
> > > inside Firefox. _Real_ correctness about checking with app can access it is
> > > not necessary in this configuration.
> > 
> > Can you grant these permissions to whatever URLs you need in desktop?  If
> > yes, then this should not break your work (except that you'd need to grant
> > the permission, of course.)
> 
> I don't really know how to grant a permission ;)

nsIPermissionManager.add: http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIPermissionManager.idl#90

Depending on the environment, you can do this in a variety of ways (in tests, you can use SpecialPowers.addPermission, or you can type in some js code into the browser console which sets it, do it in an add-on, devtools, etc.)  But you only need to do it once per URI, since permissions are persisted.
Malini and I looked into this just now, and she suggested that probably the easiest thing to do would be to run the code which uses MockContacts in the contacts app, as opposed to the system app (for example, by doing this <https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/tests/functional/contacts/test_sort_contacts.py#L21> in the Contacts app after we launch it, not before).  Dave, is that easy/possible?  

Another idea that Malini suggested was to use the the TestContainer app, run this code in that app and granting the contacts-read/write/create permissions to that app.  I think using the Contacts app itself makes more sense, but it's nice to have a backup idea too!

Dave, can you please help me here?  Thanks!
Flags: needinfo?(mdas)
Flags: needinfo?(dave.hunt)
Flags: needinfo?(21)
It's not ideal to have to open the Contacts app when adding contacts as it adds time to the tests. I think we can probably accept that if it's only for tests that insert contacts via the API, but using the TestContainer app might be preferable. Passing needinfo? onto Zac to see if he has other thoughts, or a better idea on the impact of such a change.
Flags: needinfo?(dave.hunt) → needinfo?(zcampbell)
It's not as simple as to just run the code within the Contacts app. To use the contact we've inserted, we then need to kill the Contacts app and then restart it, or refresh the page, so that the HTML loads the new data and thus we can interact with it.

We can apply contacts-read/write privileges to the System app in the test framework can't we?
Flags: needinfo?(zcampbell)
Yuren, Kevin, I don't know enough our "gaia in firefox" model, can you please look at comment 7 to 10 and tell us what's needed from our side to keep MozContacts working in this environment?
Flags: needinfo?(yurenju.mozilla)
Flags: needinfo?(kgrandon)
it's like a black magic for me, Kevin may know how it works.
Flags: needinfo?(yurenju.mozilla)
(In reply to Zac C (:zac) from comment #13)
> We can apply contacts-read/write privileges to the System app in the test
> framework can't we?

Sure.
Attachment #8465508 - Flags: review?(timdream)
About the test changes, I basically moved the real tests into iframes, which I load after I apply the permissions through SpecialPowers so that the permissions are applied before the test page is loaded.
Attachment #8464412 - Attachment is obsolete: true
Attachment #8465519 - Flags: review?(bugs)
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #17)
> Created attachment 8465508 [details] [review]
> Grant access to the contacts API from the system app

Eh, why do we want this? It's fairly trivial to script permission additions through marionette. I haven't tested this, but executing something like this should work from marionette:

host = 'app://system.gaiamobile.org';
perm = Components.classes["@mozilla.org/permissionmanager;1"]
                 .createInstance(Components.interfaces.nsIPermissionManager);
ios = Components.classes["@mozilla.org/network/io-service;1"]
                .getService(Components.interfaces.nsIIOService);
uri = ios.newURI(host, null, null);
perm.add(uri, 'contacts-write', 1);
perm.add(uri, 'contacts-read', 1);

Regarding firefox - we should be ok here. We currently load things through the app:// protocol which should load the permissions. Unfortunately lots of things are broken currently that are bigger than this so I wouldn't worry about it.
Flags: needinfo?(kgrandon)
Comment on attachment 8465519 [details] [diff] [review]
Hide the Contacts API from the contexts that lack sufficient privileges, such as Firefox desktop and Android

/me likes our webidl bindings, and the flexibility they have.


>+++ b/dom/tests/mochitest/general/test_interfaces.html
>@@ -639,19 +639,19 @@ var interfaceNamesInGlobalScope =
>     "MozApplicationEvent",
> // IMPORTANT: Do not change this list without review from a DOM peer!
>     {name: "MozCellBroadcast", b2g: true, pref: "dom.cellbroadcast.enabled"},
> // IMPORTANT: Do not change this list without review from a DOM peer!
>     {name: "MozCellBroadcastEvent", b2g: true, pref: "dom.cellbroadcast.enabled"},
> // IMPORTANT: Do not change this list without review from a DOM peer!
>     {name: "MozClirModeEvent", b2g: true, pref: "dom.mobileconnection.enabled"},
> // IMPORTANT: Do not change this list without review from a DOM peer!
>-    "mozContact",
>+    {name: "mozContact", b2g: true, permission: "contacts-read"},
> // IMPORTANT: Do not change this list without review from a DOM peer!
>-    "MozContactChangeEvent",
>+    {name: "MozContactChangeEvent", b2g: true, permission: "contacts-read"},

(Do we actually need b2b: true here. But fine)



> [NoInterfaceObject, NavigatorProperty="mozContacts",
>- JSImplementation="@mozilla.org/contactManager;1"]
>+ JSImplementation="@mozilla.org/contactManager;1",
>+ CheckPermissions="contacts-read contacts-write contacts-create"]
> interface ContactManager : EventTarget {
We should perhaps exposed only some of the method in this interface in case 
of contacts-read.
Followup?

r+, and rs+ for the test changes.
Attachment #8465519 - Flags: review?(bugs) → review+
(In reply to comment #19)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment
> #17)
> > Created attachment 8465508 [details] [review]
> > Grant access to the contacts API from the system app
> 
> Eh, why do we want this? It's fairly trivial to script permission additions
> through marionette. I haven't tested this, but executing something like this
> should work from marionette:
> 
> host = 'app://system.gaiamobile.org';
> perm = Components.classes["@mozilla.org/permissionmanager;1"]
>                  .createInstance(Components.interfaces.nsIPermissionManager);
> ios = Components.classes["@mozilla.org/network/io-service;1"]
>                 .getService(Components.interfaces.nsIIOService);
> uri = ios.newURI(host, null, null);
> perm.add(uri, 'contacts-write', 1);
> perm.add(uri, 'contacts-read', 1);

That won't work, since these permissions need to be set before the system app is launched.
See Also: → 1046937
Comment on attachment 8465508 [details] [review]
Grant access to the contacts API from the system app

I don't understand this well enough to say if this fixes anything, but speaks for review I don't think it make sense to grant permission to System app when there is no code in System app that actually use the API -- some naive future developer will try to remove it and break whether you are trying to fix with a "clean up" :P

It would be better if test environments are self-contained in their own apps -- having random bits needed in actual Gaia codebase w/o any hints may hinder development.
Attachment #8465508 - Flags: review?(timdream)
(In reply to Zac C (:zac) from comment #13)
> It's not as simple as to just run the code within the Contacts app. To use
> the contact we've inserted, we then need to kill the Contacts app and then
> restart it, or refresh the page, so that the HTML loads the new data and
> thus we can interact with it.
> 
> We can apply contacts-read/write privileges to the System app in the test
> framework can't we?

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #22)
> Comment on attachment 8465508 [details] [review]
> Grant access to the contacts API from the system app
> 
> I don't understand this well enough to say if this fixes anything, but
> speaks for review I don't think it make sense to grant permission to System
> app when there is no code in System app that actually use the API -- some
> naive future developer will try to remove it and break whether you are
> trying to fix with a "clean up" :P
> 
> It would be better if test environments are self-contained in their own apps
> -- having random bits needed in actual Gaia codebase w/o any hints may
> hinder development.

These two comments contradict each other.  I have no preferences one way or another, but without _a_ solution for these tests, I can't land my patch.  Tim, Zac, please advise which path we should take here?
Flags: needinfo?(zcampbell)
Flags: needinfo?(timdream)
(In reply to :Ehsan Akhgari (Back on 8/5 - not reading bugmail, needinfo? me!) from comment #23)
> These two comments contradict each other.  I have no preferences one way or
> another, but without _a_ solution for these tests, I can't land my patch. 
> Tim, Zac, please advise which path we should take here?

You may patch System app if this patch is schedule-sensitive and moving the tests to a self-contained app in a follow-up.

Zac, besides what's mentioned on comment 22, having tests in a self-contained app in the content process is a better CI tactic (should anything bad happened to the underlining IPC calls, and to safeguard against improper async message sequence handling etc.). What else we are testing in the System app that is not actually used in System app? That really should stop, eventually.
Flags: needinfo?(timdream)
Comment on attachment 8465508 [details] [review]
Grant access to the contacts API from the system app

OK, thanks, setting the review flag again.
Attachment #8465508 - Flags: review?(timdream)
Tim, loads of tests are using APIs at the System app level for testing.
We always just thought that all APIs would be available there and that was safe because apps would have their own permissions model. We never pursued any other avenue.
Flags: needinfo?(zcampbell)
Attachment #8465508 - Flags: review?(timdream) → review+
(In reply to Zac C (:zac) from comment #26)
> Tim, loads of tests are using APIs at the System app level for testing.
> We always just thought that all APIs would be available there and that was
> safe because apps would have their own permissions model. We never pursued
> any other avenue.

That's clearly wrong. Please find a way to address it.
You should be able to drop into chrome context from marionette and have access to all of the APIs without permissions. That's what we do in marionetteJS and it works fine.
(In reply to Kevin Grandon :kgrandon from comment #28)
> You should be able to drop into chrome context from marionette and have
> access to all of the APIs without permissions. That's what we do in
> marionetteJS and it works fine.

I tried that already, but it breaks a bunch of other things.  :/
Can someone please merge the pull request in https://bugzilla.mozilla.org/attachment.cgi?id=8465508?  Thanks!
Keywords: checkin-needed
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #29)
> (In reply to Kevin Grandon :kgrandon from comment #28)
> > You should be able to drop into chrome context from marionette and have
> > access to all of the APIs without permissions. That's what we do in
> > marionetteJS and it works fine.
> 
> I tried that already, but it breaks a bunch of other things.  :/

Sounds like we should revisit this then, but can do so in another bug. We use chrome context all over the place in gaia tests, and it generally works fine.
(In reply to Kevin Grandon :kgrandon from comment #31)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #29)
> > (In reply to Kevin Grandon :kgrandon from comment #28)
> > > You should be able to drop into chrome context from marionette and have
> > > access to all of the APIs without permissions. That's what we do in
> > > marionetteJS and it works fine.
> > 
> > I tried that already, but it breaks a bunch of other things.  :/
> 
> Sounds like we should revisit this then, but can do so in another bug. We
> use chrome context all over the place in gaia tests, and it generally works
> fine.

Hmm, actually bug 1045300 might have been enough...  Do you mind reminding me how I'd use the chrome context here please, so that I can test this again?   Thanks!
Flags: needinfo?(kgrandon)
Keywords: checkin-needed
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #32)
> Hmm, actually bug 1045300 might have been enough...  Do you mind reminding
> me how I'd use the chrome context here please, so that I can test this
> again?   Thanks!

Since the issue is with python tests, I think someone from the a-team would probably be suited to look into this. Zac - is it possible to use chrome context to create the contacts?
Flags: needinfo?(kgrandon) → needinfo?(zcampbell)
All the mechanisms are available to us, it's just a matter of whether it works. It's on my list to experiment and I'll get back to you. (keeping my ni?)
Flags: needinfo?(zcampbell)
Depends on: 1049489
I've experimented with it and conceptually it works but it will be quite a big change to make, it affects a lot of the framework because of the way it is structured.

I'm off on PTO until Friday next week so I've passed this task off to my colleagues to complete.
Latest try run: https://tbpl.mozilla.org/?tree=Try&rev=84c8b28d16d3

The only real remaining failure is gaia unit tests.
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #36)
> Latest try run: https://tbpl.mozilla.org/?tree=Try&rev=84c8b28d16d3
> 
> The only real remaining failure is gaia unit tests.

Zac, can you please help with the test changes in the above try push, or suggest someone else?  I'm stuck landing this patch at the moment...  Thanks!
Flags: needinfo?(zcampbell)
I'm told by the Sheriffs that :kgrandon is your man for this (he seems to be the man for everything these days). It's out of my area.
Flags: needinfo?(zcampbell) → needinfo?(kgrandon)
So before I was only thinking about the marionette case, we may need to think of another solution for unit tests. We would need to debug it a bit, but if the failure is because we run scripts inside of the test agent, then we may just want to grant the contacts permission to that app.

I need to investigate this a bit more, so leaving the flag on me for now.
So one option here is to try giving the test agent app contact permissions. TBH I don't know if this will actually work, but it's worth a shot.

Ehsan - Can you apply this gecko patch and push to try with it? Thanks!
Attachment #8486538 - Flags: feedback?(ehsan.akhgari)
Flags: needinfo?(kgrandon)
Thanks a lot, Kevin!  I posted it to try: <https://tbpl.mozilla.org/?tree=Try&rev=20aea6310faf>.  I will keep you posted.

BTW, I didn't know about this trick to point the try server to my own github mirror, thanks for teaching me about it! :)
Oops - I also meant to post the pull request for this change, but forgot to. The code for this branch is here: https://github.com/mozilla-b2g/gaia/pull/23865
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #41)
> Thanks a lot, Kevin!  I posted it to try:
> <https://tbpl.mozilla.org/?tree=Try&rev=20aea6310faf>.  I will keep you
> posted.
> 
> BTW, I didn't know about this trick to point the try server to my own github
> mirror, thanks for teaching me about it! :)

It looks like the same error is popping up here. Ehsan - just to confirm, is it still possible to get access to the API on desktop firefox if we grant the permission?
Flags: needinfo?(ehsan.akhgari)
Comment on attachment 8486538 [details] [diff] [review]
Try patch for gaia test runner

This fixes the error on the marionette tests on the emulator.  Thanks!
Attachment #8486538 - Flags: feedback?(ehsan.akhgari) → feedback+
Flags: needinfo?(ehsan.akhgari)
(In reply to Kevin Grandon :kgrandon from comment #43)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #41)
> > Thanks a lot, Kevin!  I posted it to try:
> > <https://tbpl.mozilla.org/?tree=Try&rev=20aea6310faf>.  I will keep you
> > posted.
> > 
> > BTW, I didn't know about this trick to point the try server to my own github
> > mirror, thanks for teaching me about it! :)
> 
> It looks like the same error is popping up here. Ehsan - just to confirm, is
> it still possible to get access to the API on desktop firefox if we grant
> the permission?

Yes, absolutely.  Do we need to grant this permission to another app as well?
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #44)
> This fixes the error on the marionette tests on the emulator.  Thanks!

Hmm, interesting. I didn't think that this would target anything outside of the Gu suite. Are we sure this is not a false-positive?

TBH I'm not really sure how Gu is being run on TBPL. Julien - do you know how we can grant the proper permissions here? Do we need to add them to the test-agent app?
Flags: needinfo?(felash)
On TBPL the test-agent is run in B2G Desktop, using --launch-app (which AFAIK just runs the application after the system and homescreen apps); I don't think this would change anything in the marionette tests.

Why do we need to grant permissions for unit tests? The tests don't rely on the proper API and use mocks (or at least they should).

On a side note, I think we should stop running the test-agent in B2G Desktop and run it in Firefox only. But the same comment applies here too.

I still don't get why the work done here is important. I'm really afraid this will break things we rely on currently (esp running Gaia and/or applications inside Firefox), and I'd like to see the improvements it brings.
Flags: needinfo?(felash)
(In reply to Julien Wajsberg [:julienw] (PTO 08/19 -> 09/15; contact schung for SMS matters) from comment #47)
> I still don't get why the work done here is important. I'm really afraid
> this will break things we rely on currently (esp running Gaia and/or
> applications inside Firefox), and I'd like to see the improvements it brings.

The reason why this is important is that it prevents the contacts API from being used in places where it's not supposed to.  There was a bug (that I can't find right now) recently where advertizing websites tried to call this API on Fennec, and prompted the user for access to their contacts.  That is unacceptable.  We need to make sure that this API is not exposed by accident.

I think I already tried to answered your question in comment 10 and the few comments before it.  Did that not answer it?
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #48)
> (In reply to Julien Wajsberg [:julienw] (PTO 08/19 -> 09/15; contact schung
> for SMS matters) from comment #47)
> > I still don't get why the work done here is important. I'm really afraid
> > this will break things we rely on currently (esp running Gaia and/or
> > applications inside Firefox), and I'd like to see the improvements it brings.
> 
> The reason why this is important is that it prevents the contacts API from
> being used in places where it's not supposed to.  There was a bug (that I
> can't find right now) recently where advertizing websites tried to call this
> API on Fennec, and prompted the user for access to their contacts.  That is
> unacceptable.  We need to make sure that this API is not exposed by accident.

Does that mean we don't want to expose this API to content ever?

> I think I already tried to answered your question in comment 10 and the few
> comments before it.  Did that not answer it?

I just come back from holidays so I still don't have time to look at this _now_ but I will before the end of this week if nobody steps up.
Flags: needinfo?(felash)
(In reply to Julien Wajsberg [:julienw] (PTO 08/19 -> 09/15; contact schung for SMS matters) from comment #49)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #48)
> > (In reply to Julien Wajsberg [:julienw] (PTO 08/19 -> 09/15; contact schung
> > for SMS matters) from comment #47)
> > > I still don't get why the work done here is important. I'm really afraid
> > > this will break things we rely on currently (esp running Gaia and/or
> > > applications inside Firefox), and I'd like to see the improvements it brings.
> > 
> > The reason why this is important is that it prevents the contacts API from
> > being used in places where it's not supposed to.  There was a bug (that I
> > can't find right now) recently where advertizing websites tried to call this
> > API on Fennec, and prompted the user for access to their contacts.  That is
> > unacceptable.  We need to make sure that this API is not exposed by accident.
> 
> Does that mean we don't want to expose this API to content ever?

That's correct.

> > I think I already tried to answered your question in comment 10 and the few
> > comments before it.  Did that not answer it?
> 
> I just come back from holidays so I still don't have time to look at this
> _now_ but I will before the end of this week if nobody steps up.

OK, thanks.
I just filed bug 1068809 so that we stop using B2G Desktop to run unit tests on TBPL (we'll still use it for integration tests obviously). I'm sick of issues with this setup, this will make things clearer.

Keeping my NI until I dig deeper to this issue.
Depends on: 1068809
Discussed on IRC, we have 2 possibilities:
* remove all dependencies to the mozContacts API in the failing tests; I'll ask the respective owners tomorrow about this
* add the chrome code to add the mozContacts permission in httpd.js; Ehsan is experimenting with this.
Depends on: 1075100
(In reply to Julien Wajsberg [:julienw] from comment #53)
> * add the chrome code to add the mozContacts permission in httpd.js; Ehsan
> is experimenting with this.

This worked!  Submitted the patch for review in bug 1075100.
Have you tried this in both Firefox and B2G Desktop?
Depends on: 1075757
Depends on: 1075770
Filed bugs to remove the dependency to mozContacts in unit tests. I hope it will move forward, otherwise ping me on IRC when I come back next week and I can have a look.
Flags: needinfo?(felash)
https://hg.mozilla.org/mozilla-central/rev/389126282bc6
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Keywords: dev-doc-needed
Chris: we also have the Navigator extension page: https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/API/Navigator

We can also move Navigator.mozContacts below it and update it (and the standard Navigator).
Flags: needinfo?(cmills)
(In reply to Jean-Yves Perrier [:teoli] from comment #61)
> Chris: we also have the Navigator extension page:
> https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/API/Navigator
> 
> We can also move Navigator.mozContacts below it and update it (and the
> standard Navigator).

Good call; done.
Flags: needinfo?(cmills)
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: