Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#7480 closed defect (fixed)

Need to 'reset' the network configurations - short term fix

Reported by: kimquirk Owned by: frances
Priority: blocker Milestone: 8.2.0 (was Update.2)
Component: sugar Version: Update.1
Keywords: blocks:8.2.0 r+ Cc: Eben, mstone, gregorio, cjb, morgs, dsd
Blocked By: Blocking:
Deployments affected: Action Needed: test in release
Verified: no

Description

Joyride 2128, q2d16

From the sugar control panel we need to be able to 'reset' or remove the stored network configuration file. This is important to do whenever someone makes a change to their AP settings, changes the password, and it might also help us debug some of the problems we are seeing with intermittent AP connections.

I think this UI could be a button in the existing network settings; and it should require a reboot.

(did I get the right simon? schampijer)

Attachments (5)

trac-7480.network_config_reset.patch (4.2 KB) - added by erikg 6 years ago.
0001-Allow-network-configuration-reset-from-control-panel.patch (3.6 KB) - added by dsd 6 years ago.
updated-network-module-for-control-panel.diff (5.0 KB) - added by Eben 6 years ago.
updated-network-module-for-control-panel-v2.diff (6.0 KB) - added by cscott 6 years ago.
Updated version of Eben's (and dsd's) patch.
updated-network-module-for-control-panel-v3.diff (5.6 KB) - added by erikos 6 years ago.
updated the patch to not come in the way of the string freeze and to fulfil the designer's requests

Download all attachments as: .zip

Change History (57)

comment:1 Changed 6 years ago by erikos

  • Owner changed from simon to erikos

nope :)

comment:2 Changed 6 years ago by erikos

  • Action Needed changed from never set to communicate
  • Cc Eben added; eben removed

comment:3 Changed 6 years ago by Eben

Is it really useful to have a "clear network associations" button? I would envision a list containing the essids of any networks that have been associated with. The list would allow deletion of the entries, and could also allow re-ordering (so as to change the order in which associations are attempted). Exposing this directly should make it rather clear what's going on.

Or, am I missing the point; are there other settings stored in the file that I didn't address?

comment:4 follow-up: Changed 6 years ago by kimquirk

Perhaps we can discuss a short term and a longer term change. In the short term we just need to remove that file and request a reboot.

In the longer term, it might be good to allow another level of detail in a future release.

comment:5 Changed 6 years ago by mstone

  • Cc gregorio added
  • Keywords 8.2.0:? added
  • Milestone changed from Never Assigned to 8.2.0 (was Update.2)

comment:6 in reply to: ↑ 4 Changed 6 years ago by mikus

Replying to kimquirk:

Perhaps we can discuss a short term and a longer term change. In the short term we just need to remove that file and request a reboot.

I'm a G1G1 user. I carry my XO around to lots of locations, each of which has their own AP. Although I have made a saved copy of my networks.cfg file, it would mean a lot of effort (e.g., asking people for keys and entering them) if that file were to disappear (and my saved copy was not up to date).

comment:7 Changed 6 years ago by erikg

  • Owner changed from erikos to erikg
  • Status changed from new to assigned

comment:8 Changed 6 years ago by erikos

I think this request came up because of #5104 - personally i think we should try to fix this instead and if we can't tell users to remove the config file manually for this release.

comment:9 follow-up: Changed 6 years ago by kimquirk

  • Priority changed from high to blocker
  • Summary changed from Need to 'reset' the network configurations to Need to 'reset' the network configurations - short term fix
  • Type changed from enhancement to defect

For 8.2, we need the ability to remove the networks.cfg file. It must be manually executed and it will remove the entire file. There can be a note that this will remove all memorized passwords; but we have to do this in order to get past some serious problems.

Independently of this, I created a ticket, #7762 to capture the longer term feature request.

This trac item should be a blocker for 8.2 since it will help G1G1 people to be more successful in connecting to their various access points. Many problems can be resolved by removing stored settings associated with having incorrectly attempted to connect to a password protected AP. Clearing this file may help.

The check box should show up in the Network Settings of the control panel. It should be labeled something like "Clear all network settings". It also needs to tell the user to reboot the laptop for the action to have affect.

TEST:

  • If you can get the bad state by click on 'olpc' (an encrypted AP) and enter incorrect password; then try the correct password. Whether it works or not (it is intermittent), you can do the next steps.
  • Go to the control panel and click on the new Network Reset checkbox.
  • Check that the file /home/olpc/.sugar/default/nm/networks.cfg is gone.
  • Reboot and try connecting again.

comment:10 in reply to: ↑ 9 Changed 6 years ago by Eben

Replying to kimquirk:

For 8.2, we need the ability to remove the networks.cfg file. It must be manually executed and it will remove the entire file. There can be a note that this will remove all memorized passwords; but we have to do this in order to get past some serious problems.

I think Simon's above comment meant manual removal a la rm networks.cfg; reboot. Has this decision been changed to require GUI?

The check box should show up in the Network Settings of the control panel. It should be labeled something like "Clear all network settings". It also needs to tell the user to reboot the laptop for the action to have affect.

This is not a checkbox. A checkbox is used to indicate persistent boolean state. This is, instead, an action button which indicates that an action should be taken when pressed. Since it's a deletion/clear operation, it should probably use the clear/destroy icon which is, at present, 'dialog-cancel' (I need to clean up the artwork theme so we have better semantic names...). The button should clearly indicate what it does: "Clear preferred networks" sounds about right to me.

TEST:

  • If you can get the bad state by click on 'olpc' (an encrypted AP) and enter incorrect password; then try the correct password. Whether it works or not (it is intermittent), you can do the next steps.
  • Go to the control panel and click on the new Network Reset checkbox.
  • Check that the file /home/olpc/.sugar/default/nm/networks.cfg is gone.
  • Reboot and try connecting again.

I'm not sure that it makes sense to be putting test cases on tickets before there is a patch to be tested against. Also, test cases (I believe) need to be indicated with "TestCase" between pipes.

comment:11 Changed 6 years ago by gregorio

  • Keywords blocks:8.2.0 added; 8.2.0:? removed

upgraded to blocks:8.2.0 per Kim's e-mail

comment:12 Changed 6 years ago by mstone

  • Keywords 8.2.0:? added

comment:13 Changed 6 years ago by erikg

The attached patch should work following http://dev.laptop.org/git?p=sugar;a=commit;h=40191c5dbd9d6b4a47a0f699d93908858acc7a50. Testing pending.

comment:14 follow-up: Changed 6 years ago by marco

  • Action Needed changed from communicate to review
  • Keywords r? added

Please follow this when submitting patches or they will sit in trac:
http://sugarlabs.org/go/DevelopmentTeam/CodeReview

comment:15 in reply to: ↑ 14 Changed 6 years ago by erikg

Replying to marco:

Please follow this when submitting patches or they will sit in trac:
http://sugarlabs.org/go/DevelopmentTeam/CodeReview

I can only view this page in a non-graphical, non-js browser. Are you testing new javascript on the sugarlabs wiki?

I've submitted the patch for review on the sugar list http://lists.laptop.org/pipermail/sugar/2008-August/007791.html.

comment:16 Changed 6 years ago by erikos

  • Keywords r- added; r? removed

I pushed the -c option for the command line interface: http://dev.laptop.org/attachment/ticket/7765/7765-reset.patch Having the name starting with 'clear' is a good thing. You might want to make the function name a bit shorter - at the moment you would have: sugar-control-panel -c networks_config_file

please use: env.get_profile_path('nm/networks.cfg') instead of: '/home/olpc/.sugar/default/nm/networks.cfg'

And have a look at the comments that morgan posted on the sugar-ml.

comment:17 Changed 6 years ago by cscott

Copying discussion from sugar@ and devel@:

On Tue, Aug 12, 2008 at 10:26:08AM +0200, Morgan Collett wrote:
> On Tue, Aug 12, 2008 at 03:31, Erik Garrison <erik@laptop.org> wrote:
> > Attached is a patch which adds a 'reset network configuration' button to
> > the network tab of the sugar control panel.  Clicking this button simply
> > rotates the config file out of the way, saving it as
> > ~/.sugar/default/nm/networks.cfg.bak.NNN  (NNN is the number of
> > previously backed-up configs +1).
> >
> > This is just a short-term fix (hack) to resolve the problem of not
> > having any gui-level method to manipulate the nm network configarion.
> > Eben has noted that we would like to enable config panel level
> > manipulation of the networks.cfg stanzas; but this requires a bit more
> > code than this immediate fix.
>
> This needs testing: in some cases NM replaces the config with what was there.
>
> I added a different AP to my home network (in parallel with my
> existing AP). To get the XOs to associate only with the new AP, I
> thought I'd simply delete networks.cfg and then associate to the new
> AP. When I rebooted to make sure it did what I wanted, networks.cfg
> had both the old and the new APs. To end up with only the new AP in
> networks.cfg, I had to first associate to the new AP, then remove the
> old one from networks.cfg - then rebooting after that showed only the
> new one.

I did test this and saw that the networks.cfg file was written again.
It appeared that NM was rewriting the config whenever it associated with
an AP.  sjored notes that NM 0.6 behaves this way, but I'm not sure it's
an issue we are trying to address here.  I think the rationale for the
reset button is the following:

   "From the sugar control panel we need to be able to 'reset' or
remove the stored network configuration file. This is important to do
whenever someone makes a change to their AP settings, changes the
password, and it might also help us debug some of the problems we are
seeing with intermittent AP connections. " ( kimquirk, trac #7480 )

The scope of this fix is pretty small.  Just make sure that stored
passwords are reset, and clear out cruft from the networks.cfg file.  Am
I missing anything?  I don't think pushing a fix into NM 0.6 is a good
use of our time, so I'm not sure what else we can do.

comment:18 Changed 6 years ago by cscott

  • Action Needed changed from review to code
  • Cc cjb added

I think we need to reset NM 0.6 in order to make this reasonable, or else people will find that "resetting their network" doesn't always work.

I believe that cjb has code in OHM to ask NM to stop NM (before suspend) and later restart it (after wakeup). My guess would be that would be sufficient. The NetworkManager-gnome interface has an "Enable Networking" checkbox which does the same thing. So: disable networking, remove networks.cfg, re-enable networking. That should be sufficient to avoid the problems Morgan pointed out.

comment:19 Changed 6 years ago by cjb

I ask NM to "sleep" and then "wake", which makes it down and then up the network interfaces it's responsible for. This seems to have bugs in it; eth0 comes back and msh0 doesn't, so I'm likely to revert it.

comment:20 Changed 6 years ago by cscott

From my reading of http://svn.gnome.org/viewvc/network-manager-applet/trunk/src/applet.c?view=annotate it seems the sleep/wake is what the "enable networking" checkbox in the GNOME applet does. Another option for OHM to try is nm_client_wireless_set_enabled...

BUT for this bug, all the saving and loading the configuration file is actually done by sugar in:

http://dev.laptop.org/git?p=sugar;a=blob;f=src/hardware/nminfo.py;h=a703ff64ea78374236ab45092e9cd8f85a237da0;hb=HEAD#l407

So it seems like all you need to do is send the appropriate sugar service a "reload configuration" signal of some sort and/or prevent the running NMInfo from writing out its config (overwriting the cleared one).

The cleanest solution that I can see is to have the NMInfoDBusServiceHelper implement an additional one-method interface (org.laptop.sugar.nm?) with a "ReloadConfig" method that does:

self._allowed_networks = self._read_config()

Then the control panel can send the org.freedesktop.NetworkManagerInfo object the ReloadConfig message after we've punted the config file.

A more forward-thinking approach might be to add a removeNetworkInfo method which could be used with the getNetworks method to remove all the remembered networks to implement a 'reset' button. In the future, when this control panel lets you edit the various network properties, you can use 'removeNetworkInfo' and 'updateNetworkInfo' to implement the edits.

comment:21 Changed 6 years ago by cscott

Incidentally, the "rewrite config file every time we associate with an AP" behavior is implemented in *sugar* (not NetworkManager) in the above nminfo.py file. But that behavior seems to be reasonable.

comment:22 Changed 6 years ago by erikos

fwiw: For radio on/off I use setWirelessEnabled/getWirelessEnabled (more info in sugar/src/controlpanel/model/network.py). That is what I figured out the right way to go with Dan.

Changed 6 years ago by erikg

comment:23 Changed 6 years ago by cscott

r-

Still doesn't fix the problem Morgan pointed out, which (as I wrote in comment 20) isn't related to toggling wireless on and off at all. You need to explicitly implement ReloadConfig or removeNetworkInfo in nminfo.py, as I wrote above.

comment:24 follow-up: Changed 6 years ago by cscott

Note that the root cause of this bug is different: nminfo.py assumes that all networks named 'linksys' are the same, and ignores the BSSID (http://en.wikipedia.org/wiki/SSID). Once you associate with one network named 'linksys' and use a password, you can not longer associate with any other network named 'linksys' with a different (or no) password.

The easiest solution is to make nminfo.py key off ESSID+BSSID, not just essid. This errs conservatively if you're in a large institution with a legimate password-protected ESSID which spans multiple BSSIDs and reprompts for the password, but this seems much safer than the current behavior.

I believe NetworkManager-gnome tries the stored password, and if that doesn't succeed, retries & reprompts for a password. That's a much more sane behavior.

I'd prefer fixing this bug for 8.2 by keying off BSSID, which doesn't require adding new UI.

comment:25 Changed 6 years ago by morgs

  • Cc morgs added

comment:26 in reply to: ↑ 24 Changed 6 years ago by erikg

Replying to cscott:

Note that the root cause of this bug is different: nminfo.py assumes that all networks named 'linksys' are the same, and ignores the BSSID (http://en.wikipedia.org/wiki/SSID). Once you associate with one network named 'linksys' and use a password, you can not longer associate with any other network named 'linksys' with a different (or no) password.

The easiest solution is to make nminfo.py key off ESSID+BSSID, not just essid. This errs conservatively if you're in a large institution with a legimate password-protected ESSID which spans multiple BSSIDs and reprompts for the password, but this seems much safer than the current behavior.
I'd prefer fixing this bug for 8.2 by keying off BSSID, which doesn't require adding new UI.

This is a classic case of not understanding the problem before jumping in to fix it. I am in error in that effort. I wish I had come to understood this specific description of the problem sooner (all linksys routers being treated similarly because of the keying used in the networks.cfg). It is not a good idea to implement a UI change to enact this. I agree that we should change nminfo.py to reflect ESSID+BISSID instead of just ESSID.

comment:27 Changed 6 years ago by kimquirk

  • Keywords 8.2.0:? removed
  • Owner changed from erikg to erikos
  • Status changed from assigned to new

I really need a solution that will make it easy for the end user to clear out network configurations. This is NOT the long term, correct solution, but a short term remove the file and reboot.

I know erikg isn't available in the next week so I need to assign this to someone else. Erikos?

comment:28 Changed 6 years ago by erikos

  • Owner changed from erikos to cscott

scott and erikg seem to agree that we should not add new interfaces and that we fix nminfi.py instead.

comment:29 Changed 6 years ago by dsd

  • Action Needed changed from code to review
  • Cc dsd added
  • Keywords r? added; r- removed

I got the impression from talking to Scott that he is OK with an extra button in the control panel to take care of this. I also feel that modifying the nmclient/nminfo code is risky in this stage of 8.2 development. And the proposal of considering both ESSID and BSSID raises difficult questions such as:

  • What if we have a key for ESSID A BSSID B, and we connect to a network with ESSID A BSSID C? Do we try to reuse the key? Should we? Security risks here..particularly if one uses shared key auth
  • What if we have a key for ESSID A BSSID B, and we have a different key for ESSID A BSSID C, and both of them are in range. Do we display both on the network view? How does the user differentiate between them?

I've attached a patch which adds a button to the control panel to clear the nminfo cache and write an updated config file.

comment:30 Changed 6 years ago by cscott

  • Keywords r- added; r? removed

Basic idea is sound. You should move the action code back into the model, though; the key seems to be *not* to use the model wrapper which is passed in to the view constructor, but instead to import controlpanel.model.network directly. Not using the model wrapper means that the 'cancel' button in the control panel frame won't actually undo your changes, though. We can probably live with that.

But please keep model & view properly separated. sugar-control-panel should be able to clear the network configuration from the command-line, too. See the sugar-update-control code for ideas if you need them.

comment:31 Changed 6 years ago by erikos

a) Hmm we should not use the gtk.STOCK_CLEAR icon. Eben which icon would you recommend. How about 'dialog-cancel'?

self._reset_config_button = gtk.Button()                                                   
self._reset_config_button.set_image(Icon(icon_name = 'dialog-cancel'))
self._reset_config_button.set_label(_('Clear'))

b) Eben, the strings get your ok as well?

c) This works only from the gui not from the command line. Any reasons for that?

d) The feedback is not terrible. I wonder if we should use an inline alert to indicate failure or success.

comment:32 Changed 6 years ago by cscott

  • Owner changed from cscott to dsd

I think that Eben should give us a proper STOCK_CLEAR icon so that we can use it in appropriate contexts, like these. If dialog-cancel is appropriate, then we should install it as the STOCK_CLEAR icon in sugar-artwork.

comment:33 Changed 6 years ago by Eben

I've added an updated presentation of the radio and network config functionality which makes it clearer what they do. Since I made teh text on the button more explicit, there is presently no icon. I think this is OK until we have a proper stock-clear (if that's dialog-ok or not).

comment:34 Changed 6 years ago by cscott

  • Keywords r? added; r- removed

I've updated Eben's patch to fix the model/view confusion. I also tweaked ModelWrapper to allow calling clear_* methods.

I'm not convinced this actually lets you clear the network configuration from the command-line client, however, because dsd's patch seems to depend on the control panel running in the same process as the sugar shell. Needs further testing/review.

Changed 6 years ago by cscott

Updated version of Eben's (and dsd's) patch.

comment:35 Changed 6 years ago by erikos

  • Keywords r- added; r? removed

I don't like that we not only add strings we change existing ones as well (radio option). I think changing the radio option should not go into the patch. Also I think we need to make it a scrolled window since the options does not fit on the screen.

Changed 6 years ago by erikos

updated the patch to not come in the way of the string freeze and to fulfil the designer's requests

comment:36 Changed 6 years ago by erikos

What happens using clear networks settings from the gui (with current patch):

  • the file and the cache gets cleared
  • you are not prompted for a sugar restart
  • going to the mesh view shows the favourite AP still with the badge and the network does still work
  • when you click on it you are prompted for the key dialogue if encrypted

comment:37 Changed 6 years ago by erikos

What happens using clear networks settings from the cmd (with current patch):

  • the file gets cleared
  • going to the mesh view shows the favourite AP still with the badge and the network does still work
  • when you click on it you are NOT prompted for the key dialogue if encrypted
  • it can connect to the AP and the old settings are written to the config file

comment:38 Changed 6 years ago by erikos

  • Keywords r? added; r- removed

Ok so we display a note for the restart of sugar when using the command line interface. So this actually works. Eben has been giving the ok for the design and the strings and we do not change existing strings.

comment:39 Changed 6 years ago by cscott

Yeah, I'd prefer to export/invoke the nminfo.clear_networks() method via dbus so that it works the same in the gui and cmd line case. That shouldn't be too big a change and is the Right Thing.

comment:40 Changed 6 years ago by marco

  • Keywords r+ added; r? removed

I agree that it's the right thing, but it's somewhat tricky. NMInfoDBusServiceHelper has a generic freedesktop naming/interface, adding our custom methods to it would be wrong. We would have to create another service, I guess

We need to get this into git asap to give a chance to translators to translate the new string. We can improve on the top of it, if we think it's worth it.

comment:41 Changed 6 years ago by marco

  • Action Needed changed from review to test in build

comment:42 Changed 6 years ago by erikos

  • Action Needed changed from test in build to approve for release

sugar-0.82.4-1.olpc3

|TestCase|

  • clear the network history in the cp network section
  • you are not prompted for a sugar restart
  • going to the mesh view shows the favourite AP still with the badge and the network does still work
  • the file ~/.sugar/default/nm/networks.cfg is empty
  • when you click on it you are prompted for the key dialogue if encrypted


comment:43 Changed 6 years ago by mstone

  • Action Needed changed from approve for release to add to release

comment:44 Changed 6 years ago by cscott

Name the specific package, please. It makes my life much easier.

comment:45 Changed 6 years ago by cscott

#8446 added for the future dbus-ification. I don't think we need to create another service; dbus has a mechanism for a service to declare several different 'interfaces' it implements. We just need to add another 'interface' to our existing service.

comment:46 Changed 6 years ago by cscott

seems to be sugar-0.82.4-1

comment:47 Changed 6 years ago by marco

Scott, yeah I thought about interfaces too. But then the service name is "org.freedesktop.NetworkManagerInfo" which is also generic. Maybe it's fine to add our own interface to it anyway, I don't know, I know very little about dbus services ABI policies/practices.

(The package name was at comment 42, perhaps we should consistenly mention in it with the switch to "approval for update", to make it easier to find it)

comment:48 Changed 6 years ago by cscott

  • Action Needed changed from add to release to test in release

sugar 0.82.5-1 added to 8.2 repo; should be in build 760 and later. Please test.

comment:50 Changed 6 years ago by mchua

  • Owner changed from dsd to frances

assigning to frances to test and verify in release candidate - see above testcase by erikos.

comment:51 Changed 6 years ago by mchua

  • Resolution set to fixed
  • Status changed from new to closed

Tested, verified, and closed on 765. I'm assuming that the last point in erikos's testcase, "when you click on it you are prompted for the key dialogue if encrypted," refers to clicking on a password-protected AP in Neighborhood view.

comment:54 Changed 5 years ago by cjb

  • Resolution set to fixed
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.