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

RESOLVED FIXED in Firefox 18

Status

()

Core
DOM: Device Interfaces
P2
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mrbkap, Assigned: chucklee)

Tracking

unspecified
B2G C2 (20nov-10dec)
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

Details

Attachments

(3 attachments, 6 obsolete attachments)

842 bytes, patch
mrbkap
: review+
Details | Diff | Splinter Review
9.77 KB, patch
Details | Diff | Splinter Review
3.19 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
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.
Blocks: 717123
Assignee: nobody → chulee
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?
Blocks: 802047
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.
blocking-basecamp: --- → ?
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.
blocking-basecamp: ? → +
Priority: -- → P2
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
};
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.
Hi Chuck, how is this bug progressing?
Flags: needinfo?(chulee)
Co-working with Gaia Team, should be done in one or two weeks.
Flags: needinfo?(chulee)
Created attachment 679082 [details] [diff] [review]
Return array of network/confignetwork data instead of object for gaia
Attachment #679082 - Flags: review?(mrbkap)
Created attachment 679083 [details] [diff] [review]
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.
Attachment #679082 - Attachment is obsolete: true
Attachment #679082 - Flags: review?(mrbkap)
Attachment #679083 - Flags: review?(mrbkap)
Created attachment 679084 [details] [diff] [review]
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.
Attachment #679084 - Flags: review?(mrbkap)
(Reporter)

Comment 11

5 years ago
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.
Attachment #679083 - Flags: review?(mrbkap)
(Reporter)

Comment 12

5 years ago
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"
Attachment #679084 - Flags: review?(mrbkap) → review+
(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.
Created attachment 679543 [details] [diff] [review]
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.
Attachment #679083 - Attachment is obsolete: true
Attachment #679543 - Flags: review?(mrbkap)
Created attachment 679544 [details] [diff] [review]
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.
Attachment #679084 - Attachment is obsolete: true
Attachment #679544 - Flags: review?(mrbkap)
Created attachment 679565 [details] [diff] [review]
0003. Expose capabilities to DOM

Expose capabilities of network to DOM, this is required for Gaia to determine the connection status.
Attachment #679565 - Flags: review?(mrbkap)
(Reporter)

Comment 17

5 years ago
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.
Attachment #679543 - Flags: review?(mrbkap) → review+
(Reporter)

Updated

5 years ago
Attachment #679544 - Flags: review?(mrbkap) → review+
(Reporter)

Updated

5 years ago
Attachment #679565 - Flags: review?(mrbkap) → review+
Created attachment 680497 [details] [diff] [review]
0001. Return array of network/confignetwork data instead of object for gaia, v2

Fix indent.
Attachment #679543 - Attachment is obsolete: true
Try server : https://tbpl.mozilla.org/?tree=Try&rev=73f5b8bc68a8
Created attachment 680500 [details] [diff] [review]
0002. Provide network connection status, v2

Resolve merge conflict
Attachment #679544 - Attachment is obsolete: true
Created attachment 680503 [details] [diff] [review]
0002. Provide network connection status, v2

Fix merge conflict
Attachment #680500 - Attachment is obsolete: true
Keywords: checkin-needed
Milestoning for C2 (deadline of 12/10), as this meets the criteria of "known P2 bugs found before or during C1".
Target Milestone: --- → B2G C2 (20nov-10dec)
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
Keywords: checkin-needed
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.
https://hg.mozilla.org/mozilla-central/rev/94e8b43c6d23
https://hg.mozilla.org/mozilla-central/rev/b1501a0a1290
https://hg.mozilla.org/mozilla-central/rev/657980dc46fd
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(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?
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.
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
status-firefox18: --- → fixed
status-firefox19: --- → fixed
You need to log in before you can comment on or make changes to this bug.