Closed
Bug 736104
Opened 13 years ago
Closed 12 years ago
WIFI: Don't treat all networks with the same SSID as equal
Categories
(Core :: DOM: Device Interfaces, defect, P2)
Tracking
()
People
(Reporter: mrbkap, Assigned: chucklee)
References
Details
Attachments
(3 files, 6 obsolete files)
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → chulee
Assignee | ||
Comment 1•12 years ago
|
||
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•12 years ago
|
||
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: --- → ?
Comment 3•12 years ago
|
||
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
Assignee | ||
Comment 4•12 years ago
|
||
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 };
Assignee | ||
Comment 5•12 years ago
|
||
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.
Assignee | ||
Comment 7•12 years ago
|
||
Co-working with Gaia Team, should be done in one or two weeks.
Flags: needinfo?(chulee)
Assignee | ||
Comment 9•12 years ago
|
||
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)
Assignee | ||
Comment 10•12 years ago
|
||
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•12 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•12 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+
Assignee | ||
Comment 13•12 years ago
|
||
(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.
Assignee | ||
Comment 14•12 years ago
|
||
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)
Assignee | ||
Comment 15•12 years ago
|
||
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)
Assignee | ||
Comment 16•12 years ago
|
||
Expose capabilities of network to DOM, this is required for Gaia to determine the connection status.
Attachment #679565 -
Flags: review?(mrbkap)
Reporter | ||
Comment 17•12 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•12 years ago
|
Attachment #679544 -
Flags: review?(mrbkap) → review+
Reporter | ||
Updated•12 years ago
|
Attachment #679565 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 20•12 years ago
|
||
Resolve merge conflict
Attachment #679544 -
Attachment is obsolete: true
Assignee | ||
Comment 21•12 years ago
|
||
Fix merge conflict
Attachment #680500 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 22•12 years ago
|
||
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)
Comment 23•12 years ago
|
||
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
Assignee | ||
Comment 24•12 years ago
|
||
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•12 years ago
|
||
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
Closed: 12 years ago
Resolution: --- → FIXED
Comment 26•12 years ago
|
||
(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?
Assignee | ||
Comment 27•12 years ago
|
||
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•12 years ago
|
||
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.
Description
•