736104 - WIFI: Don't treat all networks with the same SSID as equal

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect, P2)

Description

[Blake Kaplan (:mrbkap) (inactive)]

When scanning the list of networks, we assume that any networks with the same SSID are the same. This might not be the case, so we should match on things like security, as well.

Comment 1

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

wifi worker uses SSID as AP array index, so array index might have to change to support such function.

First thought is use SSID+Security as array index, and APs with same SSID but different security can be identified.

But gaia also uses only SSID for indexing, the modification isn't purely on gecko.

Are we sure to make certain modification?

Comment 2

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

Chunk, Evelyn and I found that we need a proper WebAPI discussion to determine the proper object keys for the returned list. Also mark as blocking? to have this to through triage.

Comment 3

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

Alternative proposal: let API return an object array, i.e. |[{ ... }, { ... }]|, where the object contained represents the network found, to avoid deciding the string we should use for the list object key.

Comment 4

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Current returned object structure by gecko, they won't(and shouldn't) be affected by the bug fix.

in WifiManager.getNetworks().onsuccess()
obj = {
  .ssid              : SSID of network
  .capabilities      : Encryption of network
                       "WPA-PSK" for WPA-PSK
                       "WPA-EAP" for WPA-EAP
                       "WEP" for WEP
                       "" for OPEN
  .bssid             : MAC of AP
  .signalStrength    : Signal strength of AP, in dBm
  .relSignalStrength : Relative signal strength of AP, range from 0 to 100
};

in WifiManager.getKnownNetworks().onsuccess()
obj = {
  .ssid         : SSID of network
  .capabilities : Encryption of network
                  "WPA-PSK" for WPA-PSK
                  "WPA-EAP" for WPA-EAP
                  "WEP" for WEP
                  "" for OPEN
  .known        : *I am not sure, seems always be true
  .identity     : Certification identity, only exist on WPA-EAP
  .password     : Certification password, for non-OPEN, only contains "*"
  .hidden       : is Hidden Network or not
};

Comment 5

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

I think it is still necessary for gecko to use object-key to manage these data, but I will transform these data into array while response to Gaia.

Comment 6

[Andrew Overholt [:overholt]]

Hi Chuck, how is this bug progressing?

Comment 7

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Co-working with Gaia Team, should be done in one or two weeks.

Comment 8

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Return array of network/confignetwork data instead of object for gaia

Comment 9

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0001. Return array of network/confignetwork data instead of object for gaia

1. Return raw data array of network/configured network for getNetwork()/getKnownNetwork, so Gaia can define how to deal with these data.

2. Change key of configured network object from "SSID" to "SSID+Security", so config of networks with same SSID but different security can be divided.

Comment 10

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0002. Provide network connection status

1. Provide |network.connected| for Gaia to determine which AP is connected.(This should already done, but seems malfunction)
2. Provide BSSID and security mode in |connection.network| for Gaia to use.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 679083 [details] [diff] [review]
0001.  Return array of network/confignetwork data instead of object for gaia

Review of attachment 679083 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good overall, but I have a few comments to address.

::: dom/wifi/WifiWorker.js
@@ +1277,5 @@
>  })();
>  
> +// Get unique key for a network, now the key is created by SSID+Security.
> +// So networks of same SSID but different security mode can be identified.
> +function getNetworkKey( network )

Nit: here and below (everywhere): no spaces before or after the parens here, please.

@@ +1284,5 @@
> +      encryption = "OPEN";
> +
> +  if ( !"ssid" in network ) {
> +    // Shouldn't be here, only if data structure changes or improper call!
> +    dump( "##### FIX ME, wifiWorker.js:getNetworkKey() expects object key \"ssid\"\n");

Here and below, I wouldn't bother checking to make sure that the object is correct. We're the ones using this function and we're responsible for doing so correctly.

@@ +1290,5 @@
> +  }
> +
> +  if ( "capabilities" in network ) {
> +    // manager network object, represents an AP
> +    // object strcutre

Nit: "structure".

@@ +1336,5 @@
> +    if ( key_mgmt == "WPA-PSK" ) {
> +      encryption = "WPA-PSK";
> +    } else if ( key_mgmt == "WPA-EAP" ) {
> +      encryption = "WPA-EAP";
> +    } else if ( key_mgmt == "NONE" && auth_alg && auth_alg === "OPEN SHARED" ) {

Why the null check on auth_alg? Null is not === a string.

@@ +1344,5 @@
> +    // Shouldn't be here, only if data structure changes or improper call!
> +    dump( "##### FIX ME, wifiWorker.js:getNetworkKey() is called with wrong object\n");
> +  }
> +
> +  return ssid + encryption;

This is safe against any ssids because encryption cannot be empty, right? So we don't need to escape or anything? Please add a small comment explaining this.

@@ +1842,5 @@
>        // scanning. Ignore any errors from this command.
>        WifiManager.setScanMode("inactive", function() {});
>        let lines = r.split("\n");
>        // NB: Skip the header line.
>        self.networks = Object.create(null);

With this patch, I don't think that networks is used anymore. You should be able to remove it.

@@ +1867,3 @@
>  
> +            var networkKey = getNetworkKey(network);
> +

Nit: Nuke this newline.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 679084 [details] [diff] [review]
0002. Provide network connection status

Review of attachment 679084 [details] [diff] [review]:
-----------------------------------------------------------------

Good catch.

::: dom/wifi/WifiWorker.js
@@ +776,5 @@
>            event.indexOf("pre-shared key may be incorrect") != -1) {
>          notify("passwordmaybeincorrect");
>        }
>  
> +      // This is ugly, but we need to grab the SSID here. BSSID is not garanteed

Nit: "guaranteed"

Comment 13

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

(In reply to Blake Kaplan (:mrbkap) from comment #11)
> Comment on attachment 679083 [details] [diff] [review]
> 0001.  Return array of network/confignetwork data instead of object for gaia
> 
> Review of attachment 679083 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good overall, but I have a few comments to address.
> 
> ::: dom/wifi/WifiWorker.js
> @@ +1344,5 @@
> > +    // Shouldn't be here, only if data structure changes or improper call!
> > +    dump( "##### FIX ME, wifiWorker.js:getNetworkKey() is called with wrong object\n");
> > +  }
> > +
> > +  return ssid + encryption;
> 
> This is safe against any ssids because encryption cannot be empty, right? So
> we don't need to escape or anything? Please add a small comment explaining
> this.
> 

I have just tried SSID of special characters <>"', it works fine. But you are right, it's safer to escape the ssid.

By the way, the regular expression used to grab associating SSID will return wrong result if there's character ' in SSID. I will try to find another expression that does the work.

Comment 14

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0001. Return array of network/confignetwork data instead of object for gaia, v2

1. Return raw data array of network/configured network for getNetwork()/getKnownNetwork, so Gaia can define how to deal with these data.

2. Change key of configured network object from "SSID" to "escape(SSID)+Security", so config of networks with same SSID but different security can be divided.

3. Remove networks object, fix coding style and typo as review comments.

Comment 15

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0002. Provide network connection status, v2

1. Provide |network.connected| for Gaia to determine which AP is connected.

2. Provide BSSID and security mode in |connection.network| for Gaia to use.

3. Change regular expression for extracting SSID to deal with special characters.

Comment 16

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0003. Expose capabilities to DOM

Expose capabilities of network to DOM, this is required for Gaia to determine the connection status.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 679543 [details] [diff] [review]
0001. Return array of network/confignetwork data instead of object for gaia, v2

Review of attachment 679543 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks.

::: dom/wifi/WifiWorker.js
@@ +1377,5 @@
> +      encryption = "WPA-PSK";
> +    } else if (key_mgmt == "WPA-EAP") {
> +      encryption = "WPA-EAP";
> +    } else if (key_mgmt == "NONE" && auth_alg === "OPEN SHARED") {
> +        encryption = "WEP";

Nit: This is misindented.

Comment 18

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0001. Return array of network/confignetwork data instead of object for gaia, v2

Fix indent.

Comment 19

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Try server : https://tbpl.mozilla.org/?tree=Try&rev=73f5b8bc68a8

Comment 20

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0002. Provide network connection status, v2

Resolve merge conflict

Comment 21

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0002. Provide network connection status, v2

Fix merge conflict

Comment 22

[Alex Keybl [:akeybl]]

Milestoning for C2 (deadline of 12/10), as this meets the criteria of "known P2 bugs found before or during C1".

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/94e8b43c6d23
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1501a0a1290
https://hg.mozilla.org/integration/mozilla-inbound/rev/657980dc46fd

Comment 24

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Sorry I forgot this patch requires modification on Gaia's Setting app, as Bug 802047.
Please hold the merge into central until Gaia completes, thanks.

Comment 25

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/94e8b43c6d23
https://hg.mozilla.org/mozilla-central/rev/b1501a0a1290
https://hg.mozilla.org/mozilla-central/rev/657980dc46fd

Comment 26

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

(In reply to Chuck Lee [:chucklee] from comment #24)
> Sorry I forgot this patch requires modification on Gaia's Setting app, as
> Bug 802047.
> Please hold the merge into central until Gaia completes, thanks.

Hum... why is this merged?

Comment 27

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

I tested it with current Gaia once I was notified of merge.

The wifi functionalities are fine, which I thought might be broken, except the presentation of "Available Network" is affected.
It is expected to be solved in Bug 802047.

As the result, the merge seems OK.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/342f6d8a8fe8
https://hg.mozilla.org/releases/mozilla-aurora/rev/8a64cba51f3b
https://hg.mozilla.org/releases/mozilla-aurora/rev/2f1fc7870844