Review Request 125723: Add options to select password storage to all password fields

Jan Grulich jgrulich at redhat.com
Tue Nov 3 09:17:50 UTC 2015 


> On Říj. 21, 2015, 7:41 dop., Heiko Tietze wrote:
> > Looks very nice. But the question is whether or not users understand the status quickly and if the dropdown menu is clear enough (the eye is a toggle button). The alternative solution is just a radio button group below the input, which is furthermore easily accessible by disabled people.
> > 
> > I'd place the "ask everytime" option as the first item since it is the default.
> 
> Jan Grulich wrote:
>     Having a radio button group below each password field would make it heavy I would say. I'm trying to remove as much visible components I can to make it more simple.
>     
>     The "ask everytime" option is not the default option, the default option should be to store secrets for the current user which should store secrets to KWallet.
> 
> Lamarque Souza wrote:
>     I agree with Jan that radio button group would pollute the ui. Talking about the default option, wouldn't be better make "Always Ask" the default for VPN connections? I know that is not consistent with the current default, but VPN is kind of a special case since usually whoever is using it is more concerned about security than convenience. By the way, good job at improving Plasma NM.
> 
> Jan Grulich wrote:
>     I agree, using "always ask" as default for VPN connections is something we should consider. 
>     
>     And thanks, plasma-nm is my passion so I don't have to force myself to make it better :).
> 
> Thomas Pfeiffer wrote:
>     I guess the source of the miscommunication between Heiko and you is that the screenshot you chose is the simples scenario possible. In the case WPA & WPA 2 Personal (PSK) there is indeed just that one field, and plenty of space to put a radio button group right in the dialog.
>     I assume what you are worried about are the more complex security setting scenarios, where the dialog is already full of fields and controls anyway, and I can understand why you don't want to put even more stuff in those dialogs.
>     
>     While I agree with Heiko that many users may not even notice the option at all, I don't think that's a big problem as long as the default option is a safe one.
>     
>     I have a few other concerns, though.
>     1. Are the passwords stored in NM encrypted? I guess not, in which case this has to be made clear to the user, because it increases the security risk. Although this makes the label quite long, I'd put "(not encrypted)" at the end, and "encrypted" behind the single-user option. Security-relevant information has to be made available to the user.
>     2. With the option "ask every time", the password is not stored at all, right? So why would I enter a password in the dialog, only to then tell the software to not store it? That does not really make sense, since this is not the place where you connect for the first time. For consistency, I'd vote for having a "store password" checkbox before the password field, like in most other places where one has the option to store a password, and to only enable the field when that checkbox is clicked
>     3. "everytime" is not an English word, it has to be "every time"
>     4. It has to be "for all users"
> 
> Jan Grulich wrote:
>     1) Yes, when they are stored in NM then they are not encrypted. I'll add the info there.
>     2) In case you select "ask every time" you don't need to put there your password because the password field will be disabled. This gets even more complicated in case where you have to specify that the password is not required at all (e.g. necessary for GSM or CDMA) so checkbox for enabling the password field probably won't work and again adds an element which doesn't need to be there, at least I would like to keep it that way.
>     3-4) I'l fix this.
> 
> Jan Grulich wrote:
>     Updated screenshot:
>     ![Image description](https://jgrulich.fedorapeople.org/nm-password-options2.png)
> 
> Heiko Tietze wrote:
>     So I can choose that the password is irrelevant? Weird :-) (just disable dropdown)
>     Since none of you responded about accessibility I'd like to bring this issue once again on the table. People who cannot use the mouse will not be able to change the type of storage. Of course, you may consider it as acceptable drawback of a nice solution.
> 
> Thomas Pfeiffer wrote:
>     The "This password is not required" option is a problem. I said that the bad discoverability of the option is not a big problem as long as nobody actually _has_ to change the option. In that case, however, it would be required to change it in order to keep the dialog from complaining that the field is empty, right? So we need to find a different solution, or we'll end up with frustrated users who cannot save their settings for a connection that does not need a password because they don't know where they can set it. 
>     
>     Actually, this is a problem with both "This password is not required" and "Ask for this password every time" (which I'd rename to "Do not store password", now that I'm reading it again): You have an option which disables a field, but it's positioned _after_ the field. Since people will go through the form from left to right, they will notice the option (if they notice it at all) only _after_ seeing the password field and thinking that they will have to fill it in. This breaks the workflow.
>     
>     Heiko is right about the potential accessibility problem. However, this is relevant not only for this option but also for the visible password option (that's relevant for a screenreader as well, isn't it?). So is there a way to access those inline buttons via keyboard?
> 
> Jan Grulich wrote:
>     We and even NM will let you to save the connection even when the password is missing but if you do not mark the password as not required, NM will probably ask for it during activation. This can be workarounded by using "not required" option as default for certain connection types (e.g. in case of gsm/cdma it would make sense). Btw. this already works in plasma-nm that way, I didn't change functionality, I just removed comboboxes and moved this options directly to the password field.
>     
>     What about putting this option to the left side of the password field? Note that comboboxes were previously also on the right side so the workflow is basically still the same.
>     
>     Heiko is unfortunately right, none of the options is accessible via keyboard. I'm not sure what we can do about this right now. A password field with an icon to show/hide the password was added officially as a new widget to KDE Frameworks so we can try to complain there and then steal the solution :D. I don't use directly the same widget because that would require dependency on latest KF5 but I implemented it the same way.
> 
> Jan Grulich wrote:
>     Ping? Can we move forward with this? I would ignore problems regarding "This password is not required" option because this was not introduced in this patch, the only change are removed comboboxes and added action icon in the password field instead. Would be adding keyboard shortcuts enough to solve the accessibility problem or does it need to be accessible using "tab" key?
> 
> Heiko Tietze wrote:
>     The inline controls have the conceptual flaw of reduced accessibility. First option: You accept it. Disabled people just have to use different methods. Or let's rather say the remaining issue goes into a new bug. Second option: Inline controls are accessible per tab (just like being separated in an extra button) and/or shortcut. This solution lacks on discoverability although. Third option: Use conservative controls such as buttons. Would be not that attractive as the inline controls. It's up to you resp. the maintainer to decide how to proceed.
>     
>     BTW: The issue is also valid and has to be solved for the visibility icon.

I don't want to of course ignore the accessibility issue, I'll try to find a solution for that. Let's forget the accessibility issue now, do you agree with this change despite it lacks on discoverability? I'm asking because I would like to push this ASAP because it's going to be message freeze soon for Plasma 5.5 and I don't know whether I'll be able or have enough time to find solution for the accessibility issue till the message freeze.


- Jan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125723/#review87177
-----------------------------------------------------------


On Říj. 21, 2015, 6:12 dop., Jan Grulich wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125723/
> -----------------------------------------------------------
> 
> (Updated Říj. 21, 2015, 6:12 dop.)
> 
> 
> Review request for Network Management, KDE Usability and Lamarque Souza.
> 
> 
> Bugs: 340707
>     http://bugs.kde.org/show_bug.cgi?id=340707
> 
> 
> Repository: plasma-nm
> 
> 
> Description
> -------
> 
> I took again inspiration from nm-connection-editor and extended our editor to allow to users select password storage. Now each password field has an option to either store password to all users (store it to NM) or to current user (store it to KWallet) or do not store it at all (user will be prompted) or tell NM that the password is not required at all (available only for some connection types). Also during transformation of strongswan vpn plugin I found out that strongswan shouldn't support password storing at all (according to NM plugin) so I removed it accordingly.
> 
> + this patch also tries to fix couple of coding style issues as usual
> 
> Screenshot:
> ![Screenshot of new options](https://jgrulich.fedorapeople.org/nm-password-options.png)
> 
> 
> Diffs
> -----
> 
>   libs/editor/connectiondetaileditor.cpp aa09d1f 
>   libs/editor/settings/bondwidget.h 2c64c31 
>   libs/editor/settings/bondwidget.cpp dbd2cf2 
>   libs/editor/settings/bridgewidget.h a322083 
>   libs/editor/settings/bridgewidget.cpp 3dc1889 
>   libs/editor/settings/btwidget.h 84a8f71 
>   libs/editor/settings/btwidget.cpp e2e9972 
>   libs/editor/settings/cdmawidget.h bbffe0d 
>   libs/editor/settings/cdmawidget.cpp f361d9e 
>   libs/editor/settings/gsmwidget.h 69d4cfd 
>   libs/editor/settings/gsmwidget.cpp 941f411 
>   libs/editor/settings/infinibandwidget.h 2c90b7f 
>   libs/editor/settings/infinibandwidget.cpp 55dfb97 
>   libs/editor/settings/ipv4widget.h a993211 
>   libs/editor/settings/ipv4widget.cpp d7e893f 
>   libs/editor/settings/ipv6widget.h 4a1f472 
>   libs/editor/settings/ipv6widget.cpp 62e96ff 
>   libs/editor/settings/pppoewidget.h 4570bba 
>   libs/editor/settings/pppoewidget.cpp 0fa4ea2 
>   libs/editor/settings/pppwidget.h 0c088ce 
>   libs/editor/settings/pppwidget.cpp 7185b7d 
>   libs/editor/settings/security802-1x.h cb5dbec 
>   libs/editor/settings/security802-1x.cpp 21929b8 
>   libs/editor/settings/teamwidget.h 39821c2 
>   libs/editor/settings/teamwidget.cpp 3db2b7a 
>   libs/editor/settings/ui/802-1x.ui f1bc058 
>   libs/editor/settings/ui/cdma.ui 9805021 
>   libs/editor/settings/ui/gsm.ui 87891cc 
>   libs/editor/settings/vlanwidget.h fabeaca 
>   libs/editor/settings/vlanwidget.cpp 5e083fa 
>   libs/editor/settings/wificonnectionwidget.h 35a59d3 
>   libs/editor/settings/wificonnectionwidget.cpp d35ec45 
>   libs/editor/settings/wifisecurity.h a263c32 
>   libs/editor/settings/wifisecurity.cpp 4d9c811 
>   libs/editor/settings/wimaxwidget.h c8f850d 
>   libs/editor/settings/wimaxwidget.cpp dcd212c 
>   libs/editor/settings/wiredconnectionwidget.h ed8dc88 
>   libs/editor/settings/wiredconnectionwidget.cpp d8fcd90 
>   libs/editor/settings/wiredsecurity.h a34731e 
>   libs/editor/settings/wiredsecurity.cpp 8fdb1cc 
>   libs/editor/widgets/passwordfield.h 1bd21a2 
>   libs/editor/widgets/passwordfield.cpp 05fe6dc 
>   libs/editor/widgets/settingwidget.h fdae197 
>   vpn/l2tp/l2tp.ui b04ebce 
>   vpn/l2tp/l2tpauth.h 54b6462 
>   vpn/l2tp/l2tpauth.cpp 7369458 
>   vpn/l2tp/l2tpwidget.h 1e4c383 
>   vpn/l2tp/l2tpwidget.cpp b278228 
>   vpn/openconnect/openconnectauth.h 9a77421 
>   vpn/openconnect/openconnectauth.cpp fecb16e 
>   vpn/openconnect/openconnectwidget.h ae0e9d2 
>   vpn/openconnect/openconnectwidget.cpp c293e52 
>   vpn/openswan/openswan.ui a6d61fa 
>   vpn/openswan/openswanauth.h aa78f88 
>   vpn/openswan/openswanauth.cpp 1fd79fb 
>   vpn/openswan/openswanwidget.h 55a54dd 
>   vpn/openswan/openswanwidget.cpp b94f752 
>   vpn/openvpn/openvpn.ui 1c49cad 
>   vpn/openvpn/openvpnadvanced.ui ce89ce1 
>   vpn/openvpn/openvpnadvancedwidget.h c0346ee 
>   vpn/openvpn/openvpnadvancedwidget.cpp 0864e35 
>   vpn/openvpn/openvpnauth.h dd3b564 
>   vpn/openvpn/openvpnauth.cpp 5250bc1 
>   vpn/openvpn/openvpnwidget.h 51d7aab 
>   vpn/openvpn/openvpnwidget.cpp e27e216 
>   vpn/pptp/pptpauth.h 1772c81 
>   vpn/pptp/pptpauth.cpp 8ffec08 
>   vpn/pptp/pptpprop.ui c713f24 
>   vpn/pptp/pptpwidget.h 2b25dd7 
>   vpn/pptp/pptpwidget.cpp 880d6c5 
>   vpn/ssh/sshauth.h 88dcebc 
>   vpn/ssh/sshauth.cpp 6e8ffa9 
>   vpn/ssh/sshwidget.h fa3f852 
>   vpn/ssh/sshwidget.cpp 7b3ad28 
>   vpn/ssh/sshwidget.ui 1d0300b 
>   vpn/sstp/sstpauth.h 08f5025 
>   vpn/sstp/sstpauth.cpp b452a73 
>   vpn/sstp/sstpwidget.h 0156b86 
>   vpn/sstp/sstpwidget.cpp 7ff68aa 
>   vpn/sstp/sstpwidget.ui 51dfc96 
>   vpn/strongswan/strongswanauth.h d23947d 
>   vpn/strongswan/strongswanauth.cpp f034abe 
>   vpn/strongswan/strongswanprop.ui 06fb254 
>   vpn/strongswan/strongswanwidget.h 7204ff5 
>   vpn/strongswan/strongswanwidget.cpp 094e81e 
>   vpn/vpnc/vpnc.ui fc83bf8 
>   vpn/vpnc/vpncauth.h eb24744 
>   vpn/vpnc/vpncauth.cpp 9142682 
>   vpn/vpnc/vpncwidget.h 9aa4ac8 
>   vpn/vpnc/vpncwidget.cpp 6aaffda 
> 
> Diff: https://git.reviewboard.kde.org/r/125723/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jan Grulich
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20151103/f77c9d4e/attachment-0001.html>

More information about the kde-networkmanager mailing list