Use DOM API to enable/disable Wifi hotspot.

The reason to provide these APIs is to remove access setting in Wifi component in gecko. Remove access setting can provide more clear interface from gecko. Following is proposed API for wifi tethering: /** * Enable/Disable wifi tethering. * @param enable True to enable wifi tethering, False to disable wifi tethering. * @param configuration configuration should have the following fields when enable is True: * - ssid SSID network name. * - securityType open, wpa-psk or wpa2-psk. * - securityPassword password for wpa-psk or wpa2-psk. * - ip ip address. * - prefix mask length. * - dhcpStartIp start ip address allocated by DHCP server for wifi tethering. * - dhcpEndIp end ip address allocated by DHCP server for wifi tethering. * - dns1 first DNS server address. * - dns2 second DNS server address. * - usbDhcpStartIp start ip address allocated by DHCP server for usb tethering. * - usbDhcpEndIp end ip address allocated by DHCP server for usb tethering. * configuration should be null when enable is False. * onsuccess: We have successfully enable/disable Wifi tethering. * onerror: We have failed to enable/disable Wifi tethering. */ nsIDOMDOMRequest setWifiTetheringaEnable(in boolean enable, in jsval configuration); /** * Update wifi tethering configuration. * @param configuration configuration should have at least one of following fields. * - ssid SSID network name. * - securityType open, wpa-psk or wpa2-psk. * - securityPassword password for wpa-psk or wpa2-psk. * - ip ip address. * - prefix mask length. * - dhcpStartIp start ip address allocated by DHCP server for wifi tethering. * - dhcpEndIp end ip address allocated by DHCP server for wifi tethering. * - dns1 first DNS server address. * - dns2 second DNS server address. * - usbDhcpStartIp start ip address allocated by DHCP server for usb tethering. * - usbDhcpEndIp end ip address allocated by DHCP server for usb tethering. * onsuccess: We have successfully update wifi tethering configuration. * onerror: We have failed to update wifi tethering configuration. */ nsIDOMDOMRequest setWifiTetheringaConfiguration(in jsval configuration); /** * Return current wifi tethering configuration. */ readonly attribute jsval wifiTetheringConfiguration; Usage: 1. When GAIA apps want to enable/disable wifi tethering, call setWifiTetheringaEnable with tethering parameter. 2. When wifi tethering parameter is changed, call setWifiTetheringaConfiguration. If current tethering state is on, gecko will restart wifi. If current tethering state is off, gecko will only update the tethering configuration 3. When GAIA apps need to display tethering parameter, access the wifiTetheringConfiguration jsval.

Hi Arthur, The API change will also need modification from GAIA, could you help check if the modification needed in GAIA is reasonable ? or do you have any concern ? Thanks. The modification needed was written in Comment 1 Usage, the main modification will be GAIA need to call setWifiTetheringaConfiguration API if there are any tethering parameters list in the API doc changed. Hi Blake, Could you help check if proposed DOM API is OK or do you have any suggestion ? Thanks for help.

Looks good to me. I was wondering is the related configuration kept in gecko after rebooting? Just want to make sure the behavior.

(In reply to Arthur Chen [:arthurcc] from comment #3) > Looks good to me. I was wondering is the related configuration kept in gecko > after rebooting? Just want to make sure the behavior. No, gecko will not keep the configuration after rebooting. In tethering-enabled case, after rebooting, GAIA will need to call setWifiTetheringaEnable with previous configuration.

This might be a silly question, but can we change the tethering configuration without turning it off and then on? If not, I'm not sure I understand why we'd prefer offering a "set...Configuration" API instead of having Gaia just turn it off and then on with the new configuration. Otherwise, I'm fine with that part of it. With the proposed API, which code is responsible for making sure that wifi and tethering aren't turned on at the same time? Are we going to push that into Gaia?

(In reply to Dimi Lee [:dlee] from comment #4) > (In reply to Arthur Chen [:arthurcc] from comment #3) > > Looks good to me. I was wondering is the related configuration kept in gecko > > after rebooting? Just want to make sure the behavior. > > No, gecko will not keep the configuration after rebooting. > In tethering-enabled case, after rebooting, GAIA will need to call > setWifiTetheringaEnable with previous configuration. I am not sure if Gaia have such bootstrap-alike procedure now. If not, I think it's OK to load initial parameters from settings while wifi manager is first initialized.

(In reply to Blake Kaplan (:mrbkap) from comment #5) > This might be a silly question, but can we change the tethering > configuration without turning it off and then on? If not, I'm not sure I > understand why we'd prefer offering a "set...Configuration" API instead of > having Gaia just turn it off and then on with the new configuration. > Otherwise, I'm fine with that part of it. > > With the proposed API, which code is responsible for making sure that wifi > and tethering aren't turned on at the same time? Are we going to push that > into Gaia? I prefer check it in gecko and return req.onerror if conflict, and leave the error handling to Gaia. That's what I like to do in bug 930355, the enable API for wifi.

(In reply to Blake Kaplan (:mrbkap) from comment #5) > This might be a silly question, but can we change the tethering > configuration without turning it off and then on? If not, I'm not sure I > understand why we'd prefer offering a "set...Configuration" API instead of > having Gaia just turn it off and then on with the new configuration. > Otherwise, I'm fine with that part of it. > > With the proposed API, which code is responsible for making sure that wifi > and tethering aren't turned on at the same time? Are we going to push that > into Gaia? We cannot change tethering configuration without turning off then on. After checking the code and discuss with Arthur, indeed we do not need setWifiTetheringaEnable API and either wifiTetheringConfiguration. I will propose for gecko there is only one API setWifiTetheringaEnable to disable/enable tethering. I will write it in Comment 9

Following is proposed API for wifi tethering: /** * Enable/Disable wifi tethering. * @param enable True to enable wifi tethering, False to disable wifi tethering. * @param configuration configuration should have the following fields when enable is True: * - ssid SSID network name. * - securityType open, wpa-psk or wpa2-psk. * - securityPassword password for wpa-psk or wpa2-psk. * - ip ip address. * - prefix mask length. * - dhcpStartIp start ip address allocated by DHCP server for wifi tethering. * - dhcpEndIp end ip address allocated by DHCP server for wifi tethering. * - dns1 first DNS server address. * - dns2 second DNS server address. * - usbDhcpStartIp start ip address allocated by DHCP server for usb tethering. * - usbDhcpEndIp end ip address allocated by DHCP server for usb tethering. * configuration should be null when enable is False. * onsuccess: We have successfully enable/disable Wifi tethering. * onerror: We have failed to enable/disable Wifi tethering. */ nsIDOMDOMRequest setWifiTetheringaEnable(in boolean enable, in jsval configuration); Modification for GAIA : 1. To enable/disable wifi-tethering, GAIA apps should use setWifiTetheringaEnable API. Gecko will not react to setting change. 2. During bootstrap, GAIA will need to read wifi-tethering settings and enable tethering if necessary. 3. If tethering parameter is changed, GAIA should disable wifi tethering and then enable it with new configuration. 4. GAIA will be responsible for handling Wifi state and Wifi-Tethering state conflict. Gecko will return error if the status is incorrect.

Hi Arthur, Blake Please help revise the proposal in Comment 9 , thanks!

(In reply to Chuck Lee [:chucklee] from comment #7) > I prefer check it in gecko and return req.onerror if conflict, and leave the > error handling to Gaia. > That's what I like to do in bug 930355, the enable API for wifi. This sounds good to me as well. (In reply to Dimi Lee [:dlee] from comment #9) > nsIDOMDOMRequest setWifiTetheringaEnable(in boolean enable, in jsval > configuration); Watch out for the typo in the function name! I'd call this: setWifiTetheringEnabled(...) The rest of this sounds good to me.

Looks good to me. Thanks.

Hi EJ, Just want to let you know that GECKO will provide patch to adopt this change soon. Detail information for GAIA could be found in Comment 9. Please feel free to let me know if you have any question or concern. Thanks

Hi Dimi, is this API backward compatible ? After you land this patch, will it break codes using the old way to turn on/off tethering ? Currently from gaia side, we may have no other resources for wifi related API changes because we already planed what to do in next release. Thanks !!

Hi EJ, We will keep old way(configure by setting) until GAIA using new API, so it won't break anything :)

Ok ! In this way, I think it is ok to do the change from my side ! Just want to ask few questions about this API, 1. Why Gaia has to pass such a complicated configuration to Gecko to turn on/off tethering ? Currently we only get some basic information like ssid, ip and security type (I think). 2. I have never seen prefix, dhcpStartIp, dhcpEndIp, dns1, dns2, usbDhcpStartIp, usbDhcpEndIp ... before, where can we get this ? 3. Can we just pass some easy information like ssid, security type and ip(?) to let Gecko know which network we are going to used for tethering ? Thanks :)

(In reply to EJ Chen [:eragonj][:小龍哥] from comment #16) > Ok ! In this way, I think it is ok to do the change from my side ! > > Just want to ask few questions about this API, > > 1. Why Gaia has to pass such a complicated configuration to Gecko to turn > on/off tethering ? Currently we only get some basic information like ssid, > ip and security type (I think). > > 2. I have never seen prefix, dhcpStartIp, dhcpEndIp, dns1, dns2, > usbDhcpStartIp, usbDhcpEndIp ... before, where can we get this ? > > 3. Can we just pass some easy information like ssid, security type and ip(?) > to let Gecko know which network we are going to used for tethering ? > > Thanks :) For the API we list all possible configurations for WIFI tethering, and every configuration has a default value. So it is ok that just configure SSID, security type and password, and then gecko will use all default value for all other un-configured setting.

Dimi, thanks for the clarification, it helps a lot !

We are able to write a test case for this patch like this [1]. Dimi, please let me know if you have a hard time writing the test case. I am glad to help you. Thanks! [1] http://hg.mozilla.org/mozilla-central/file/e1a037c085d1/dom/wifi/test/marionette/test_wifi_tethering_wifi_active.js

(In reply to Henry Chang [:henry] from comment #20) > We are able to write a test case for this patch like this [1]. > > Dimi, please let me know if you have a hard time writing the test case. > I am glad to help you. Thanks! > > [1] > http://hg.mozilla.org/mozilla-central/file/e1a037c085d1/dom/wifi/test/ > marionette/test_wifi_tethering_wifi_active.js Oh, thanks! I forget we already have test case for Wifi now. I will add a test case for this.

Comment on attachment 8454299 [details] [diff] [review] Bug 936357 patch v1 Review of attachment 8454299 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/webidl/MozWifiManager.webidl @@ +136,5 @@ > + DOMString? wifiEndIp; > + DOMString? dns1; > + DOMString? dns2; > + DOMString? usbStartIp; > + DOMString? usbEndIp; They should not be nullable, these parameters are mandatory for hotspot. @@ +281,5 @@ > > /** > + * Enable/Disable WIFI tethering. > + * @param enabled True to enable WIFI tethering, False to disable WIFI tethering. > + * @param config configuration may have following fields when enable is True: Basically, application "should" provide these configuration. We should define the usage of the API more strictly in the comment. Although, both of us agree to put the default values in gecko in case some of these values are not provided by application. ::: dom/wifi/DOMWifiManager.js @@ +524,5 @@ > + this._sendMessageForRequest("WifiManager:setWifiTethering", > + { > + enabled: enabled, > + config: config > + }, request); Nit: "{" should follow the coding style in this file. ::: dom/wifi/WifiWorker.js @@ +3541,5 @@ > }.bind(this)); > } > }, > > + handleWifiTetheringEnabled: function(enabled, callback) { Do we need to notify the gaia when hotspot is controlled by wifi toggle in notifyTetheringOn() and notifyTetheringOff()?

Hi Blake, After discussing with :vchang, we would like to have a centralized tethering API which could support wifi, usb and bluetooth tethering. Attachment is the draft for the proposed webidl, please let me know if you have any concern or suggestion, thanks!

Comment on attachment 8460132 [details] [diff] [review] (WIP)TetheringManager webidl proposal Review of attachment 8460132 [details] [diff] [review]: ----------------------------------------------------------------- This seems reasonable to me. ::: dom/webidl/MozTetheringManager.webidl @@ +16,5 @@ > + > +dictionary WifiTetheringConfig { > + DOMString ssid; > + SecurityType security; > + DOMString key; This should be nullable for the "open" case, right?

Hi Blake, One more question for the centralize tethering API. In this case we assume this API will support both bt and wifi tethering in the future. So it means for APP to use this API, it should have both bt and wifi permission even it will only use one of them. Do you think this is reasonable ?

I think that's reasonable, but I'm basing that on the idea that I can't think of an app where we would grant either the bt or wifi permission without the other one. Paul, does that make sense to you as well?

Question: in the latest IDL proposal it looks like you don't support a way to read the tethering configuration? Or am I reading it wrong. How do you support this use case then: > > 2. During bootstrap, GAIA will need to read wifi tethering settings and enable tethering if necessary. > Either way, my feeling is that we are better off creating a new permission name here: e.g. "tethering" or similar. We _could_ check both bluetooth and wifi-manage permissions instead, but there is a push to expose a subset of both bluetooth and wifi-manage to privileged apps, and it wont be obvious that granting both bluetooth & wifi to an app also allows access to the tethering API. I could be convinced otherwise, but unless these is a good reason to combine these permissions, it might be more flexible to keep this separate.

(In reply to Paul Theriault [:pauljt] from comment #28) > Question: in the latest IDL proposal it looks like you don't support a way > to read the tethering configuration? Or am I reading it wrong. How do you > support this use case then: > > > > 2. During bootstrap, GAIA will need to read wifi tethering settings and enable tethering if necessary. > > The tethering configuration will be stored in GAIA, gecko will not store this information. For tethering status, there is another bug 927298. > > Either way, my feeling is that we are better off creating a new permission > name here: e.g. "tethering" or similar. > > We _could_ check both bluetooth and wifi-manage permissions instead, but > there is a push to expose a subset of both bluetooth and wifi-manage to > privileged apps, and it wont be obvious that granting both bluetooth & wifi > to an app also allows access to the tethering API. I could be convinced > otherwise, but unless these is a good reason to combine these permissions, > it might be more flexible to keep this separate. So do you suggest that we check "tethering", "wifi" and "bt" permission in the beginning or check only "tethering" permission in the beginning and when app use setWifiTetheringEnabled API then we will check the permission according to the tethering type in parameter ?

Actually will this be a certified only API? Maybe instead of a 'tethering' permission this should just have a line like: http://mxr.mozilla.org/mozilla-central/source/dom/webidl/ResourceStats.webidl#9 to limit access to certified apps only. And then yes I agree you should do a permission check according to the tethering type (e.g. if type==wifi, then check for wifi-manage, similarly for bluetooth.) What would you check for USB though? I don't think we support this yet though so maybe worry about this once it is supported?

(In reply to Paul Theriault [:pauljt] from comment #30) > Actually will this be a certified only API? Maybe instead of a 'tethering' > permission this should just have a line like: > > http://mxr.mozilla.org/mozilla-central/source/dom/webidl/ResourceStats. > webidl#9 > If this API should be a certified API is also a problem bothers us now. For now, the answer will be yes, but in the future, could this be a privilege or even a non-privilege API ? Do you know who can provide some suggestion for this ? > to limit access to certified apps only. And then yes I agree you should do a > permission check according to the tethering type (e.g. if type==wifi, then > check for wifi-manage, similarly for bluetooth.) > > What would you check for USB though? I don't think we support this yet > though so maybe worry about this once it is supported? Yes we will check USB after we implement it.

After internal discussion, this API will be a certified API. So i will implement it as paul mentioned in Comment 30

Note. Blake mentioned in Comment 25 for nullable for the "open" case. If set WifiTetheringConfig to nullable it will show following compile error : "Dictionary TetheringConfiguration has member with nullable dictionary type"

(In reply to Dimi Lee[:dimi][:dlee] from comment #31) > (In reply to Paul Theriault [:pauljt] from comment #30) > > Actually will this be a certified only API? Maybe instead of a 'tethering' > > permission this should just have a line like: > > > > http://mxr.mozilla.org/mozilla-central/source/dom/webidl/ResourceStats. > > webidl#9 > > > > If this API should be a certified API is also a problem bothers us now. For > now, the answer will be yes, but in the future, could this be a privilege or > even a non-privilege API ? This question gets asked of me a lot. Its not a question of "is this allowed". The question is how do you propose that we make it safe for an arbitrary website to change your wifi/bluetooth settings. Also I can imagine a third-party wifi manager that may want to use be able to control this, but what is the use case for allowing regular web apps (or websites) to be able to do this. > > Do you know who can provide some suggestion for this ? I can provide guidance around the types of security threats. The main threat here is being able to change the devices network settings. If we want to expose this privileged apps we should probably look at 'wife-manage' as a whole since the implications are similar. But there is a fair amount of risk involved here for what would seem to be a limited use case? > > > to limit access to certified apps only. And then yes I agree you should do a > > permission check according to the tethering type (e.g. if type==wifi, then > > check for wifi-manage, similarly for bluetooth.) > > > > What would you check for USB though? I don't think we support this yet > > though so maybe worry about this once it is supported? > Yes we will check USB after we implement it.

Comment on attachment 8467626 [details] [diff] [review] Part1. Add TetheringManager webidl v1 Review of attachment 8467626 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me.

Comment on attachment 8467626 [details] [diff] [review] Part1. Add TetheringManager webidl v1 Review of attachment 8467626 [details] [diff] [review]: ----------------------------------------------------------------- I'm going to wait for the reviewers to have a go before reviewing here, but I did notice one thing. ::: dom/webidl/MozTetheringManager.webidl @@ +50,5 @@ > + * - security open, wpa-psk or wpa2-psk. > + * - key password for wpa-psk or wpa2-psk. > + * config should not be set when enabled is False. > + */ > + Promise setTetheringEnabled(boolean enabled, This is going to need to be updated for <https://groups.google.com/forum/#!topic/mozilla.dev.platform/HcCvSw4wJMM>.

Comment on attachment 8467626 [details] [diff] [review] Part1. Add TetheringManager webidl v1 Review of attachment 8467626 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Thank you.

Comment on attachment 8467628 [details] [diff] [review] Part3. WifiWorker support setWifiTethering API v1 Review of attachment 8467628 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/wifi/WifiWorker.js @@ +3436,5 @@ > + if ((enabled && WifiManager.tetheringState == "COMPLETED") || > + (!enabled && WifiManager.tetheringState == "UNINITIALIZED")) { > + self._sendMessage(message, true, msg.data, msg); > + } else { > + msg.data.reason = enabled ? "Enable WIFI tethering faild" : "Disable WIFI tethering faild"; Nit: 80 characters per line.

Comment on attachment 8467627 [details] [diff] [review] Part2. TetheringManager DOM implementation Review of attachment 8467627 [details] [diff] [review]: ----------------------------------------------------------------- The patch looks good to me, but I am not quite familiar with promise request. So f+. ::: dom/tethering/TetheringManager.js @@ +71,5 @@ > + > + setTetheringEnabled: function setTetheringEnabled(aEnabled, aType, aConfig) { > + let self = this; > + switch (aType) { > + case "wifi": Please define these to const. For instance, const TETHERING_TYPE_WIFI = "wifi";

- Rebase latest code - Address review's comment

try server result: https://tbpl.mozilla.org/?tree=Try&rev=e8c5f0f70aed