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

Jan Grulich jgrulich at redhat.com
Wed Oct 21 06:08:09 UTC 2015 


> On Říj. 20, 2015, 5:27 odp., Lamarque Souza wrote:
> > libs/editor/widgets/passwordfield.h, line 32
> > <https://git.reviewboard.kde.org/r/125723/diff/1/?file=411857#file411857line32>
> >
> >     In Plasma NM 0.9.0.x we used to have an option to store secrets for a list of users. Has not that option been ported to Plasma NM >= 0.9.3.x?

Nope and I don't even know that there was such option. I also don't know how to store secrets for a list of users because you don't have access to their KWallet and if you store secrets to NM then they will be available for everyone.


> On Říj. 20, 2015, 5:27 odp., Lamarque Souza wrote:
> > libs/editor/widgets/passwordfield.h, line 40
> > <https://git.reviewboard.kde.org/r/125723/diff/1/?file=411857#file411857line40>
> >
> >     I think this should be renamed to setPasswordOptionsEnabled, like what is done in Qt.

Right, I had before enablePasswordOptions() and then I decided to make it as setter and just blindly add "set" at the beginning without thinking :).


> On Říj. 20, 2015, 5:27 odp., Lamarque Souza wrote:
> > libs/editor/widgets/passwordfield.h, line 41
> > <https://git.reviewboard.kde.org/r/125723/diff/1/?file=411857#file411857line41>
> >
> >     This sounds really wierd. Maybe you can join this and the method above into one:
> >     
> >     setOptionEnabled(PasswordOption option, bool enable);
> >     
> >     That would make the API cleaner.

The first one enables all options, the second enables just certain option because not requiring password doesn't make sense in some cases.


> On Říj. 20, 2015, 5:27 odp., Lamarque Souza wrote:
> > libs/editor/widgets/passwordfield.cpp, line 49
> > <https://git.reviewboard.kde.org/r/125723/diff/1/?file=411858#file411858line49>
> >
> >     Well, the password is not stored for all users, it is stored once and is available to all users. I am not able to find a short and accurate description for that. Is "Store password and make it available to all users" ok here?

Yes, seems to be ok.


> On Říj. 20, 2015, 5:27 odp., Lamarque Souza wrote:
> > libs/editor/widgets/passwordfield.cpp, line 77
> > <https://git.reviewboard.kde.org/r/125723/diff/1/?file=411858#file411858line77>
> >
> >     What is "the position we want"? I mean, why remove and then readd the action?

We want it (the action with password options represented by icon) to be on the right side right behind the "show password action" which is not if it's added additionally so we have to remove the existing action first. I tried to just make the action with password options hidden or disabled and be there by default but it's visible all the time.


- Jan


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


On Říj. 20, 2015, 11:29 dop., Jan Grulich wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125723/
> -----------------------------------------------------------
> 
> (Updated Říj. 20, 2015, 11:29 dop.)
> 
> 
> Review request for Network Management 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/20151021/4d859c86/attachment-0001.html>

More information about the kde-networkmanager mailing list