GNOME Bugzilla – Bug 646454
[patch] don't show hidden APs in the menu
Last modified: 2011-09-09 23:35:03 UTC
If it doesn't have an SSID, there's not a good way to connect to it without asking for the SSID, and we need a good design for doing that. The problem is that you need to know the SSID *and* all security details before you try to connect, because you can't scan for the AP and autodetect its security (because you don't know the SSID yet). So we should be dropping hidden SSIDs from the menu and handling them specially later, either from the menu vai "connect to hidden" item or from the CC panel.
Created attachment 184881 [details] [review] hide hidden APs
Review of attachment 184881 [details] [review]: - Generally we prefer patches as git-format-patch (git-bz) formatted patches with a commit message - You could remove the ssid.length == 0 check from here, since the => UTF-8 cnversion would be "" which is false (and NMNetworkMenuItem._init() depends on this), but doesn't hurt to have it like this But generally seems to make sense to me and looks safe assuming that the SSID of an access point can't change (and you'd know better than me on that issue). Will assign to Giovanni for his review.
Upon review of the NM code, it looks like APs can change their SSID later if they get "found" (ie, the user manually requests a connection to a hidden SSID, which causes the wifi driver to probe-scan the SSID, which makes the SSID then show up). So the patch isn't technically correct; nm-applet gets around this by constructing the menu every time it's displayed, which means it doesn't need to care about the SSID changing underneath it.
(In reply to comment #3) > Upon review of the NM code, it looks like APs can change their SSID later if > they get "found" (ie, the user manually requests a connection to a hidden SSID, > which causes the wifi driver to probe-scan the SSID, which makes the SSID then > show up). So the patch isn't technically correct; nm-applet gets around this > by constructing the menu every time it's displayed, which means it doesn't need > to care about the SSID changing underneath it. When implementing this, I just thought that all NMAccessPoint objects exposed by libnm-glib where visible APs with an SSID, and added the <unknown> fallback for SSIDs that could not be converted to utf8, so I'm not sure how do best do it here. One option is that if the ssid is null or empty (the !NetworkManager.utils_ssid_to_utf8 is wrong here, it removes valid APs with non utf8 ssids), we connect to notify::ssid and call again _accessPointAdded. (This requires NetworkManager emitting PropertiesChanged for Ssid and libnm-glib converting it to notify signals)
(In reply to comment #4) > (In reply to comment #3) > > Upon review of the NM code, it looks like APs can change their SSID later if > > they get "found" (ie, the user manually requests a connection to a hidden SSID, > > which causes the wifi driver to probe-scan the SSID, which makes the SSID then > > show up). So the patch isn't technically correct; nm-applet gets around this > > by constructing the menu every time it's displayed, which means it doesn't need > > to care about the SSID changing underneath it. > > When implementing this, I just thought that all NMAccessPoint objects exposed > by libnm-glib where visible APs with an SSID, and added the <unknown> fallback > for SSIDs that could not be converted to utf8, so I'm not sure how do best do > it here. > One option is that if the ssid is null or empty (the > !NetworkManager.utils_ssid_to_utf8 is wrong here, it removes valid APs with non > utf8 ssids), we connect to notify::ssid and call again _accessPointAdded. (This > requires NetworkManager emitting PropertiesChanged for Ssid and libnm-glib > converting it to notify signals) Check the code for utils_ssid_to_utf8 - it's really aggressive about falling back and not failing - I don't think, in fact, think it ever will return a "" or NULL result unless the input is "" or NULL.
"So we should be dropping hidden SSIDs from the menu and handling them specially later, either from the menu vai "connect to hidden" item or from the CC panel." When you say 'later'...this is something that really needs to be fixed soon, as quite a lot of people seem to run APs with hidden SSIDs and they need to have a way to connect to them. I'd feel happier if this had a schedule and not a hand-wave... (see downstream bug, https://bugzilla.redhat.com/show_bug.cgi?id=691139 )
(In reply to comment #6) > "So we should be dropping hidden SSIDs from the menu and handling them > specially > later, either from the menu vai "connect to hidden" item or from the CC panel." > > When you say 'later'...this is something that really needs to be fixed soon, as > quite a lot of people seem to run APs with hidden SSIDs and they need to have a > way to connect to them. I'd feel happier if this had a schedule and not a > hand-wave... > > (see downstream bug, https://bugzilla.redhat.com/show_bug.cgi?id=691139 ) Actually, there was a "Connect to hidden wireless network..." in earlier iterations of the network menu. It was removed after comments from the designers, but it can be readded without much trouble (using nm-applet from 3.0)
(In reply to comment #7) > (In reply to comment #6) > > "So we should be dropping hidden SSIDs from the menu and handling them > > specially > > later, either from the menu vai "connect to hidden" item or from the CC panel." > > > > When you say 'later'...this is something that really needs to be fixed soon, as > > quite a lot of people seem to run APs with hidden SSIDs and they need to have a > > way to connect to them. I'd feel happier if this had a schedule and not a > > hand-wave... > > > > (see downstream bug, https://bugzilla.redhat.com/show_bug.cgi?id=691139 ) > > Actually, there was a "Connect to hidden wireless network..." in earlier > iterations of the network menu. It was removed after comments from the > designers, but it can be readded without much trouble (using nm-applet from > 3.0) Hidden networks are pretty common, just removing the entry feel very wrong to me.
OK, clearly too late for design changes at this point. What's the state of the patch? From reading the comments, it seems like if you connect to a hidden AP via the control panel, it will still not show up in the menu?
(In reply to comment #9) > OK, clearly too late for design changes at this point. What's the state of the > patch? From reading the comments, it seems like if you connect to a hidden AP > via the control panel, it will still not show up in the menu? Current status: all APs are shown, the hidden ones with "<unknown>". After connecting via the control panel, the menu is refreshed (due to addConnection and _deviceStateChanged) and the SSID is shown. Current status with patch: APs that are initially hidden are not tracked at all by the menu, and will never be shown, unless they are removed and added again by the network card. Connecting via the panel will have no effect. At the very least, connecting to notify::ssid in _accessPointAdded instead of just dropping the AP on the floor is needed. Also, we should remove the debug log in _accessPointRemoved, as it becomes legitimate to remove access points that were never actually added.
(In reply to comment #10) > Current status: all APs are shown, the hidden ones with "<unknown>" which is not great... > Current status with patch: APs that are initially hidden are not tracked at all > by the menu, and will never be shown ...but that's definitely worse. So we should probably keep the status quo for 3.0.1.
Created attachment 189090 [details] [review] NetworkMenu: don't show hidden access points It is not possible to connect to hidden access points without knowing the SSID, and it should be done using the control center panel and the appropriate dialog. At the same time, this should fix some warnings from libnm-glib and dbus-glib.
It seems pretty clunky to force people to go through the control panel for this; can't the network list in the shell icon just have an 'Other...' entry at the bottom, like the one in the control panel applet does?
Comment on attachment 189090 [details] [review] NetworkMenu: don't show hidden access points didn't test it, but it looks mostly right. OK to commit if it's tested. >+ this._activeConnectionItem = new NMNetworkMenuItem(this._activeNetwork.accessPoints, undefined, > { reactive: false }); >+ this._activeConnectionItem = new NMNetworkMenuItem(this._activeNetwork.accessPoints, undefined, > { reactive: false but fix the indentation of the second-and-later lines in both of these calls while you're there
(In reply to comment #14) > (From update of attachment 189090 [details] [review]) > didn't test it, but it looks mostly right. OK to commit if it's tested. Not yet, I don't have hidden APs here (and my router doesn't let me change the SSID or make it hidden)
*** Bug 650680 has been marked as a duplicate of this bug. ***
Attachment 189090 [details] pushed as 6709e5e - NetworkMenu: don't show hidden access points Its tested status did not change, but I think it's worth pushing, given that hidden (or anyway with null SSID) access points are affecting many people, and have prompted incomplete patches in the past.
So, my 'dupe' of this - https://bugzilla.gnome.org/show_bug.cgi?id=650680 - is a very visible bug in Fedora 15. The large patch in this bug doesn't backport at all cleanly to GNOME Shell 3.0.2 (as present in F15) - it seems like there's a lot of changes to the code between 3.0 and 3.1 here. So even though this is 'fixed', a very visible case of it is still present in F15. Is there any chance of having Shell 3.0 fixed wrt this - at least the specific sub-case in 650680? I could just apply the small patch that was proposed in 650680 to Fedora's gnome-shell package, I suppose, but that doesn't seem ideal.