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

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Contacts, defect)

Description

[(no longer active)]

We don't seem to have any code that checks it.

Comment 1

[Reuben Morais [:reuben]]

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.

Comment 2

[(no longer active)]

(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.

Comment 3

[(no longer active)]

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!

Comment 4

[(no longer active)]

(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...

Comment 5

[(no longer active)]

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?

Comment 6

[(no longer active)]

Attachment: Hide the Contacts API from the contexts that lack sufficient privileges, such as Firefox desktop and Android (WIP)

Comment 7

[Julien Wajsberg [:julienw]]

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.

Comment 8

[(no longer active)]

(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.)

Comment 9

[Julien Wajsberg [:julienw]]

(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...

Comment 10

[(no longer active)]

(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.

Comment 11

[(no longer active)]

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!

Comment 12

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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.

Comment 13

[Zac C (:zac)]

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?

Comment 14

[Julien Wajsberg [:julienw]]

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?

Comment 15

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

it's like a black magic for me, Kevin may know how it works.

Comment 16

[(no longer active)]

(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.

Comment 17

[(no longer active)]

Attachment: Grant access to the contacts API from the system app

Comment 18

[(no longer active)]

Attachment: Hide the Contacts API from the contexts that lack sufficient privileges, such as Firefox desktop and Android

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.

Comment 19

[Kevin Grandon :kgrandon]

(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.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 21

[(no longer active)]

(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.

Comment 22

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 23

[(no longer active)]

(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?

Comment 24

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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.

Comment 25

[(no longer active)]

Comment on attachment 8465508 [details] [review]
Grant access to the contacts API from the system app

OK, thanks, setting the review flag again.

Comment 26

[Zac C (:zac)]

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.

Comment 27

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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.

Comment 28

[Kevin Grandon :kgrandon]

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.

Comment 29

[(no longer active)]

(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.  :/

Comment 30

[(no longer active)]

Can someone please merge the pull request in https://bugzilla.mozilla.org/attachment.cgi?id=8465508?  Thanks!

Comment 31

[Kevin Grandon :kgrandon]

(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.

Comment 32

[(no longer active)]

(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!

Comment 33

[Kevin Grandon :kgrandon]

(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?

Comment 34

[Zac C (:zac)]

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?)

Comment 35

[Zac C (:zac)]

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.

Comment 36

[(no longer active)]

Latest try run: https://tbpl.mozilla.org/?tree=Try&rev=84c8b28d16d3

The only real remaining failure is gaia unit tests.

Comment 37

[(no longer active)]

(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!

Comment 38

[Zac C (:zac)]

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.

Comment 39

[Kevin Grandon :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.

Comment 40

[Kevin Grandon :kgrandon]

Attachment: Try patch for gaia test runner

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!

Comment 41

[(no longer active)]

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! :)

Comment 42

[Kevin Grandon :kgrandon]

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

Comment 43

[Kevin Grandon :kgrandon]

(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?

Comment 44

[(no longer active)]

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!

Comment 45

[(no longer active)]

(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?

Comment 46

[Kevin Grandon :kgrandon]

(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?

Comment 47

[Julien Wajsberg [:julienw]]

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.

Comment 48

[(no longer active)]

(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?

Comment 49

[Julien Wajsberg [:julienw]]

(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.

Comment 50

[(no longer active)]

(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.

Comment 51

[Julien Wajsberg [:julienw]]

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.

Comment 52

[(no longer active)]

New try push: https://tbpl.mozilla.org/?tree=Try&rev=185bc3597224

Comment 53

[Julien Wajsberg [:julienw]]

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.

Comment 54

[(no longer active)]

(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.

Comment 55

[Julien Wajsberg [:julienw]]

Have you tried this in both Firefox and B2G Desktop?

Comment 56

[Julien Wajsberg [:julienw]]

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.

Comment 57

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/389126282bc6

Comment 58

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/389126282bc6

Comment 59

[Kohei Yoshino]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/firefox-os-apis-have-been-hidden-from-web-content/

Comment 60

[Chris Mills [:cmills]]

The API ref docs have already been moved into the Firefox OS APIs section:

https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/API/MozContact
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/API/MozContactChangeEvent
https://developer.mozilla.org/en-US/docs/Web/API/Navigator/mozContacts

I've also added a note to the Firefox 48 release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/48#Others

Comment 61

[Jean-Yves Perrier [:teoli]]

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).

Comment 62

[Chris Mills [: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.