D20930: Update WireGuard to match NetworkManager 1.16 interface

[Jan Grulich]

This revision was automatically updated to reflect the committed changes.
Closed by commit R116:751f4da01b42: Update WireGuard to match NetworkManager 
1.16 interface (authored by jgrulich).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D20930?vs=57983&id=58052#toc

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20930?vs=57983&id=58052

REVISION DETAIL
  https://phabricator.kde.org/D20930

AFFECTED FILES
  applet/contents/ui/ConnectionItem.qml
  kcm/kcm.cpp
  kcm/qml/ConnectionItem.qml
  kded/notification.cpp
  kded/secretagent.cpp
  libs/declarative/connectionicon.cpp
  libs/declarative/networkstatus.cpp
  libs/declarative/networkstatus.h
  libs/editor/CMakeLists.txt
  libs/editor/connectioneditorbase.cpp
  libs/editor/settings/connectionwidget.cpp
  libs/editor/settings/ui/wireguardinterfacewidget.ui
  libs/editor/settings/ui/wireguardpeerwidget.ui
  libs/editor/settings/ui/wireguardtabwidget.ui
  libs/editor/settings/wireguardinterfacewidget.cpp
  libs/editor/settings/wireguardinterfacewidget.h
  libs/editor/settings/wireguardpeerwidget.cpp
  libs/editor/settings/wireguardpeerwidget.h
  libs/editor/settings/wireguardtabwidget.cpp
  libs/editor/settings/wireguardtabwidget.h
  libs/editor/wireguardkeyvalidator.cpp
  libs/editor/wireguardkeyvalidator.h
  libs/models/creatableconnectionsmodel.cpp
  libs/models/networkmodelitem.cpp
  libs/uiutils.cpp
  libs/uiutils.h
  vpn/CMakeLists.txt
  vpn/wireguard/CMakeLists.txt
  vpn/wireguard/Messages.sh
  vpn/wireguard/nm-wireguard-service.h
  vpn/wireguard/plasmanetworkmanagement_wireguardui.desktop
  vpn/wireguard/wireguard.cpp
  vpn/wireguard/wireguard.h
  vpn/wireguard/wireguard.ui
  vpn/wireguard/wireguardadvanced.ui
  vpn/wireguard/wireguardadvancedwidget.cpp
  vpn/wireguard/wireguardadvancedwidget.h
  vpn/wireguard/wireguardauth.cpp
  vpn/wireguard/wireguardauth.h
  vpn/wireguard/wireguardauth.ui
  vpn/wireguard/wireguardkeyvalidator.cpp
  vpn/wireguard/wireguardkeyvalidator.h
  vpn/wireguard/wireguardwidget.cpp
  vpn/wireguard/wireguardwidget.h

To: andersonbruce, jgrulich, #vdg, ngraham
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Jan Grulich]

jgrulich accepted this revision.
jgrulich added a comment.
This revision is now accepted and ready to land.


  Good work!!

REPOSITORY
  R116 Plasma Network Management Applet

BRANCH
  TwoTabs

REVISION DETAIL
  https://phabricator.kde.org/D20930

To: andersonbruce, jgrulich, #vdg, ngraham
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

andersonbruce added a comment.


  The changes to the key passwordfield menus was made to remove the AlwaysAsk 
option. This is the last change I am aware of necessary to release this.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D20930

To: andersonbruce, jgrulich, #vdg, ngraham
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

andersonbruce updated this revision to Diff 57983.
andersonbruce added a comment.


  - Disable "AlwaysAsk" option in key password fields
  
  Caution: will not compile without changes to passwordfield.cpp and
  
passwordfield.h made in the current master branch.

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20930?vs=57972&id=57983

BRANCH
  TwoTabs

REVISION DETAIL
  https://phabricator.kde.org/D20930

AFFECTED FILES
  applet/contents/ui/ConnectionItem.qml
  kcm/kcm.cpp
  kcm/qml/ConnectionItem.qml
  kded/notification.cpp
  kded/secretagent.cpp
  libs/declarative/connectionicon.cpp
  libs/declarative/networkstatus.cpp
  libs/declarative/networkstatus.h
  libs/editor/CMakeLists.txt
  libs/editor/connectioneditorbase.cpp
  libs/editor/settings/connectionwidget.cpp
  libs/editor/settings/ui/wireguardinterfacewidget.ui
  libs/editor/settings/ui/wireguardpeerwidget.ui
  libs/editor/settings/ui/wireguardtabwidget.ui
  libs/editor/settings/wireguardinterfacewidget.cpp
  libs/editor/settings/wireguardinterfacewidget.h
  libs/editor/settings/wireguardpeerwidget.cpp
  libs/editor/settings/wireguardpeerwidget.h
  libs/editor/settings/wireguardtabwidget.cpp
  libs/editor/settings/wireguardtabwidget.h
  libs/editor/wireguardkeyvalidator.cpp
  libs/editor/wireguardkeyvalidator.h
  libs/models/creatableconnectionsmodel.cpp
  libs/models/networkmodelitem.cpp
  libs/uiutils.cpp
  libs/uiutils.h
  vpn/CMakeLists.txt
  vpn/wireguard/CMakeLists.txt
  vpn/wireguard/Messages.sh
  vpn/wireguard/nm-wireguard-service.h
  vpn/wireguard/plasmanetworkmanagement_wireguardui.desktop
  vpn/wireguard/wireguard.cpp
  vpn/wireguard/wireguard.h
  vpn/wireguard/wireguard.ui
  vpn/wireguard/wireguardadvanced.ui
  vpn/wireguard/wireguardadvancedwidget.cpp
  vpn/wireguard/wireguardadvancedwidget.h
  vpn/wireguard/wireguardauth.cpp
  vpn/wireguard/wireguardauth.h
  vpn/wireguard/wireguardauth.ui
  vpn/wireguard/wireguardkeyvalidator.cpp
  vpn/wireguard/wireguardkeyvalidator.h
  vpn/wireguard/wireguardwidget.cpp
  vpn/wireguard/wireguardwidget.h

To: andersonbruce, jgrulich, #vdg, ngraham
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Jan Grulich]

jgrulich added a comment.


  I think it doesn't matter in this case, you can just change it in your code, 
you don't need to have that change in your local copy, if this is merged than 
it's applied on top of that so it will be ok.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D20930

To: andersonbruce, jgrulich, #vdg, ngraham
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

andersonbruce marked an inline comment as done.
andersonbruce added a comment.


  In D20930#464412 , @jgrulich wrote:
  
  > In D20930#464403 , 
@andersonbruce wrote:
  >
  > > In D20930#464387 , @jgrulich 
wrote:
  > >
  > > > In D20930#464384 , @jgrulich 
wrote:
  > > >
  > > > > In D20930#464377 , 
@andersonbruce wrote:
  > > > >
  > > > > > Updated SecretAgent class to always try to get the secrets from 
kwallet even if the 'allow-interaction' flag is set. The keys WireGuard uses 
are 43 random characters long and we don't expect the user to enter these 
manually when trying to make a connection. If data is not available in kwallet 
then trying to make a connection will fail. Also updated the configuration 
screens to not allow a configuration with "AlwaysAsk" flags on either key.
  > > > >
  > > > >
  > > > > You can disable "AlwaysAsk" option with 
PasswordField::setPasswordNotRequiredEnabled(false).
  > > >
  > > >
  > > > Sorry, that's not it, I added a new option to do that, you can now use 
PasswordField::setPasswordNotSavedEnabled(false).
  > >
  > >
  > > Great!  Should I make the change in this review? If so, what is the 
correct procedure (with git, arc, ...) to merge that change into this review?
  >
  >
  > Should be enough just to merge current master into your branch I guess.
  
  
  Isn't  'arc diff' going to try to include the differences between the master 
I originally checked out and the merged current master as changes in the 
WireGuard review then?  Sorry if these are stupid questions, I'm not a git 
expert and I just don't want to mess things up.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D20930

To: andersonbruce, jgrulich, #vdg, ngraham
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Jan Grulich]

jgrulich added a comment.


  In D20930#464403 , @andersonbruce 
wrote:
  
  > In D20930#464387 , @jgrulich 
wrote:
  >
  > > In D20930#464384 , @jgrulich 
wrote:
  > >
  > > > In D20930#464377 , 
@andersonbruce wrote:
  > > >
  > > > > Updated SecretAgent class to always try to get the secrets from 
kwallet even if the 'allow-interaction' flag is set. The keys WireGuard uses 
are 43 random characters long and we don't expect the user to enter these 
manually when trying to make a connection. If data is not available in kwallet 
then trying to make a connection will fail. Also updated the configuration 
screens to not allow a configuration with "AlwaysAsk" flags on either key.
  > > >
  > > >
  > > > You can disable "AlwaysAsk" option with 
PasswordField::setPasswordNotRequiredEnabled(false).
  > >
  > >
  > > Sorry, that's not it, I added a new option to do that, you can now use 
PasswordField::setPasswordNotSavedEnabled(false).
  >
  >
  > Great!  Should I make the change in this review? If so, what is the correct 
procedure (with git, arc, ...) to merge that change into this review?
  
  
  Should be enough just to merge current master into your branch I guess.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D20930

To: andersonbruce, jgrulich, #vdg, ngraham
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

andersonbruce marked an inline comment as done.
andersonbruce added a comment.


  
  
  In D20930#464387 , @jgrulich wrote:
  
  > In D20930#464384 , @jgrulich 
wrote:
  >
  > > In D20930#464377 , 
@andersonbruce wrote:
  > >
  > > > Updated SecretAgent class to always try to get the secrets from kwallet 
even if the 'allow-interaction' flag is set. The keys WireGuard uses are 43 
random characters long and we don't expect the user to enter these manually 
when trying to make a connection. If data is not available in kwallet then 
trying to make a connection will fail. Also updated the configuration screens 
to not allow a configuration with "AlwaysAsk" flags on either key.
  > >
  > >
  > > You can disable "AlwaysAsk" option with 
PasswordField::setPasswordNotRequiredEnabled(false).
  >
  >
  > Sorry, that's not it, I added a new option to do that, you can now use 
PasswordField::setPasswordNotSavedEnabled(false).
  
  
  Great!  Should I make the change in this review? If so, what is the correct 
procedure (with git, arc, ...) to merge that change into this review?

INLINE COMMENTS

> jgrulich wrote in secretagent.cpp:432
> Shouldn't it be
> 
>   (isVpn && !isWireguard) && allowInteraction
> 
> You don't want to display password dialog in case it's a WG connection.
> 
> Or maybe it should stay and instead we should return true and send error 
> (like we do when a dialog has an error) if it's a WG connection, to let 
> NetworkManager know we cannot handle that.

Actually what I did is move the WireGuard handling above this 'else if' so it 
shouldn't get to this point if it is WireGuard so I simply returned the logic 
to what it was originally.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D20930

To: andersonbruce, jgrulich, #vdg, ngraham
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

andersonbruce updated this revision to Diff 57972.
andersonbruce added a comment.


  - Return non-WireGuard logic to original state

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20930?vs=57970&id=57972

BRANCH
  TwoTabs

REVISION DETAIL
  https://phabricator.kde.org/D20930

AFFECTED FILES
  applet/contents/ui/ConnectionItem.qml
  kcm/kcm.cpp
  kcm/qml/ConnectionItem.qml
  kded/notification.cpp
  kded/secretagent.cpp
  libs/declarative/connectionicon.cpp
  libs/declarative/networkstatus.cpp
  libs/declarative/networkstatus.h
  libs/editor/CMakeLists.txt
  libs/editor/connectioneditorbase.cpp
  libs/editor/settings/connectionwidget.cpp
  libs/editor/settings/ui/wireguardinterfacewidget.ui
  libs/editor/settings/ui/wireguardpeerwidget.ui
  libs/editor/settings/ui/wireguardtabwidget.ui
  libs/editor/settings/wireguardinterfacewidget.cpp
  libs/editor/settings/wireguardinterfacewidget.h
  libs/editor/settings/wireguardpeerwidget.cpp
  libs/editor/settings/wireguardpeerwidget.h
  libs/editor/settings/wireguardtabwidget.cpp
  libs/editor/settings/wireguardtabwidget.h
  libs/editor/wireguardkeyvalidator.cpp
  libs/editor/wireguardkeyvalidator.h
  libs/models/creatableconnectionsmodel.cpp
  libs/models/networkmodelitem.cpp
  libs/uiutils.cpp
  libs/uiutils.h
  vpn/CMakeLists.txt
  vpn/wireguard/CMakeLists.txt
  vpn/wireguard/Messages.sh
  vpn/wireguard/nm-wireguard-service.h
  vpn/wireguard/plasmanetworkmanagement_wireguardui.desktop
  vpn/wireguard/wireguard.cpp
  vpn/wireguard/wireguard.h
  vpn/wireguard/wireguard.ui
  vpn/wireguard/wireguardadvanced.ui
  vpn/wireguard/wireguardadvancedwidget.cpp
  vpn/wireguard/wireguardadvancedwidget.h
  vpn/wireguard/wireguardauth.cpp
  vpn/wireguard/wireguardauth.h
  vpn/wireguard/wireguardauth.ui
  vpn/wireguard/wireguardkeyvalidator.cpp
  vpn/wireguard/wireguardkeyvalidator.h
  vpn/wireguard/wireguardwidget.cpp
  vpn/wireguard/wireguardwidget.h

To: andersonbruce, jgrulich, #vdg, ngraham
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Jan Grulich]

jgrulich added a comment.


  In D20930#464384 , @jgrulich wrote:
  
  > In D20930#464377 , 
@andersonbruce wrote:
  >
  > > Updated SecretAgent class to always try to get the secrets from kwallet 
even if the 'allow-interaction' flag is set. The keys WireGuard uses are 43 
random characters long and we don't expect the user to enter these manually 
when trying to make a connection. If data is not available in kwallet then 
trying to make a connection will fail. Also updated the configuration screens 
to not allow a configuration with "AlwaysAsk" flags on either key.
  >
  >
  > You can disable "AlwaysAsk" option with 
PasswordField::setPasswordNotRequiredEnabled(false).
  
  
  Sorry, that's not it, I added a new option to do that, you can now use 
PasswordField::setPasswordNotSavedEnabled(false).

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D20930

To: andersonbruce, jgrulich, #vdg, ngraham
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Jan Grulich]

jgrulich added inline comments.

INLINE COMMENTS

> secretagent.cpp:432
> +return true;
> +} else if (requestNew || (allowInteraction && 
> !setting->needSecrets(requestNew).isEmpty()) || (allowInteraction && 
> userRequested) || ((isVpn || isWireGuard) && allowInteraction)) {
> +

Shouldn't it be

  (isVpn && !isWireguard) && allowInteraction

You don't want to display password dialog in case it's a WG connection.

Or maybe it should stay and instead we should return true and send error (like 
we do when a dialog has an error) if it's a WG connection, to let 
NetworkManager know we cannot handle that.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D20930

To: andersonbruce, jgrulich, #vdg, ngraham
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Jan Grulich]

jgrulich added a comment.


  In D20930#464377 , @andersonbruce 
wrote:
  
  > Updated SecretAgent class to always try to get the secrets from kwallet 
even if the 'allow-interaction' flag is set. The keys WireGuard uses are 43 
random characters long and we don't expect the user to enter these manually 
when trying to make a connection. If data is not available in kwallet then 
trying to make a connection will fail. Also updated the configuration screens 
to not allow a configuration with "AlwaysAsk" flags on either key.
  
  
  You can disable "AlwaysAsk" option with 
PasswordField::setPasswordNotRequiredEnabled(false).

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D20930

To: andersonbruce, jgrulich, #vdg, ngraham
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

andersonbruce added a comment.


  Updated SecretAgent class to always try to get the secrets from kwallet even 
if the 'allow-interaction' flag is set. The keys WireGuard uses are 43 random 
characters long and we don't expect the user to enter these manually when 
trying to make a connection. If data is not available in kwallet then trying to 
make a connection will fail. Also updated the configuration screens to not 
allow a configuration with "AlwaysAsk" flags on either key.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D20930

To: andersonbruce, jgrulich, #vdg, ngraham
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

andersonbruce updated this revision to Diff 57970.
andersonbruce added a comment.


  - Don't allow save of configuration with "AlwaysAsk" flag for keys

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20930?vs=57969&id=57970

BRANCH
  TwoTabs

REVISION DETAIL
  https://phabricator.kde.org/D20930

AFFECTED FILES
  applet/contents/ui/ConnectionItem.qml
  kcm/kcm.cpp
  kcm/qml/ConnectionItem.qml
  kded/notification.cpp
  kded/secretagent.cpp
  libs/declarative/connectionicon.cpp
  libs/declarative/networkstatus.cpp
  libs/declarative/networkstatus.h
  libs/editor/CMakeLists.txt
  libs/editor/connectioneditorbase.cpp
  libs/editor/settings/connectionwidget.cpp
  libs/editor/settings/ui/wireguardinterfacewidget.ui
  libs/editor/settings/ui/wireguardpeerwidget.ui
  libs/editor/settings/ui/wireguardtabwidget.ui
  libs/editor/settings/wireguardinterfacewidget.cpp
  libs/editor/settings/wireguardinterfacewidget.h
  libs/editor/settings/wireguardpeerwidget.cpp
  libs/editor/settings/wireguardpeerwidget.h
  libs/editor/settings/wireguardtabwidget.cpp
  libs/editor/settings/wireguardtabwidget.h
  libs/editor/wireguardkeyvalidator.cpp
  libs/editor/wireguardkeyvalidator.h
  libs/models/creatableconnectionsmodel.cpp
  libs/models/networkmodelitem.cpp
  libs/uiutils.cpp
  libs/uiutils.h
  vpn/CMakeLists.txt
  vpn/wireguard/CMakeLists.txt
  vpn/wireguard/Messages.sh
  vpn/wireguard/nm-wireguard-service.h
  vpn/wireguard/plasmanetworkmanagement_wireguardui.desktop
  vpn/wireguard/wireguard.cpp
  vpn/wireguard/wireguard.h
  vpn/wireguard/wireguard.ui
  vpn/wireguard/wireguardadvanced.ui
  vpn/wireguard/wireguardadvancedwidget.cpp
  vpn/wireguard/wireguardadvancedwidget.h
  vpn/wireguard/wireguardauth.cpp
  vpn/wireguard/wireguardauth.h
  vpn/wireguard/wireguardauth.ui
  vpn/wireguard/wireguardkeyvalidator.cpp
  vpn/wireguard/wireguardkeyvalidator.h
  vpn/wireguard/wireguardwidget.cpp
  vpn/wireguard/wireguardwidget.h

To: andersonbruce, jgrulich, #vdg, ngraham
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

andersonbruce updated this revision to Diff 57969.
andersonbruce marked an inline comment as done.
andersonbruce added a comment.


  - Always get encrypted data from kwallet
  
  Don't ask user for key values.

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20930?vs=57888&id=57969

BRANCH
  TwoTabs

REVISION DETAIL
  https://phabricator.kde.org/D20930

AFFECTED FILES
  applet/contents/ui/ConnectionItem.qml
  kcm/kcm.cpp
  kcm/qml/ConnectionItem.qml
  kded/notification.cpp
  kded/secretagent.cpp
  libs/declarative/connectionicon.cpp
  libs/declarative/networkstatus.cpp
  libs/declarative/networkstatus.h
  libs/editor/CMakeLists.txt
  libs/editor/connectioneditorbase.cpp
  libs/editor/settings/connectionwidget.cpp
  libs/editor/settings/ui/wireguardinterfacewidget.ui
  libs/editor/settings/ui/wireguardpeerwidget.ui
  libs/editor/settings/ui/wireguardtabwidget.ui
  libs/editor/settings/wireguardinterfacewidget.cpp
  libs/editor/settings/wireguardinterfacewidget.h
  libs/editor/settings/wireguardpeerwidget.cpp
  libs/editor/settings/wireguardpeerwidget.h
  libs/editor/settings/wireguardtabwidget.cpp
  libs/editor/settings/wireguardtabwidget.h
  libs/editor/wireguardkeyvalidator.cpp
  libs/editor/wireguardkeyvalidator.h
  libs/models/creatableconnectionsmodel.cpp
  libs/models/networkmodelitem.cpp
  libs/uiutils.cpp
  libs/uiutils.h
  vpn/CMakeLists.txt
  vpn/wireguard/CMakeLists.txt
  vpn/wireguard/Messages.sh
  vpn/wireguard/nm-wireguard-service.h
  vpn/wireguard/plasmanetworkmanagement_wireguardui.desktop
  vpn/wireguard/wireguard.cpp
  vpn/wireguard/wireguard.h
  vpn/wireguard/wireguard.ui
  vpn/wireguard/wireguardadvanced.ui
  vpn/wireguard/wireguardadvancedwidget.cpp
  vpn/wireguard/wireguardadvancedwidget.h
  vpn/wireguard/wireguardauth.cpp
  vpn/wireguard/wireguardauth.h
  vpn/wireguard/wireguardauth.ui
  vpn/wireguard/wireguardkeyvalidator.cpp
  vpn/wireguard/wireguardkeyvalidator.h
  vpn/wireguard/wireguardwidget.cpp
  vpn/wireguard/wireguardwidget.h

To: andersonbruce, jgrulich, #vdg, ngraham
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Jan Grulich]

jgrulich added inline comments.

INLINE COMMENTS

> andersonbruce wrote in connectionicon.cpp:403
> There is still a discrepancy between the enum defined by NetworkManager and 
> the one defined by NMQT.  NetworkManager is returning a value of 29 but 
> NetworkManager::Device::WireGuard has a value of 30.

This is correct, we are unfortunately +1 because back then there was a device 
which we add support for and later during the NM development cycle it was 
renamed and I couldn't just simply rename or remove an enum, because it would 
break ABI. You can see in NMQT that there is code converting NM  device type to 
NMQT device type so we this coverd.

> andersonbruce wrote in wireguardpeerwidget.cpp:52
> SimpleIpV4AddressValidator and SimpleIpV6AddressValidator were intentionally 
> done this way because they were existing functions that I didn't want to 
> break when I added new (defaulted) parameters for use in with WireGuard 
> functionality.
> 
> Since changing these two will involve changes to non-WireGuard code and it 
> makes sense to do them all at the same time, can we write this up as a 
> separate Bug?

Ok, we can do that in a separate review.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D20930

To: andersonbruce, jgrulich, #vdg, ngraham
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

andersonbruce marked 8 inline comments as done.
andersonbruce added a comment.


  Update to fix some comments.

INLINE COMMENTS

> jgrulich wrote in connectionicon.cpp:403
> You can now use NetworkManager::Device::WireGuard.

There is still a discrepancy between the enum defined by NetworkManager and the 
one defined by NMQT.  NetworkManager is returning a value of 29 but 
NetworkManager::Device::WireGuard has a value of 30.

> jgrulich wrote in wireguardpeerwidget.cpp:52
> You can change all the validators to take all the necessary parameters as 
> first and then have a default value for the parent object so you don't need 
> to pass a nullptr.
> 
> E.g.
> 
>   explicit SimpleIpV4AddressValidator(AddressStyle style = 
> AddressStyle::Base, QObject *parent = nullptr);
> 
> I should have noticed this before. Can you change all the validators you use 
> this way?

SimpleIpV4AddressValidator and SimpleIpV6AddressValidator were intentionally 
done this way because they were existing functions that I didn't want to break 
when I added new (defaulted) parameters for use in with WireGuard functionality.

Since changing these two will involve changes to non-WireGuard code and it 
makes sense to do them all at the same time, can we write this up as a separate 
Bug?

> jgrulich wrote in networkmodelitem.cpp:480
> There is already WireGuard device in NMQT with KF5 5.58 which you should be 
> able to use.

This was just an extraneous comment so it was removed

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D20930

To: andersonbruce, jgrulich, #vdg, ngraham
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

andersonbruce updated this revision to Diff 57888.
andersonbruce added a comment.


  - Correct review comments

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20930?vs=57790&id=57888

BRANCH
  TwoTabs

REVISION DETAIL
  https://phabricator.kde.org/D20930

AFFECTED FILES
  applet/contents/ui/ConnectionItem.qml
  kcm/kcm.cpp
  kcm/qml/ConnectionItem.qml
  kded/notification.cpp
  kded/secretagent.cpp
  libs/declarative/connectionicon.cpp
  libs/declarative/networkstatus.cpp
  libs/declarative/networkstatus.h
  libs/editor/CMakeLists.txt
  libs/editor/connectioneditorbase.cpp
  libs/editor/settings/connectionwidget.cpp
  libs/editor/settings/ui/wireguardinterfacewidget.ui
  libs/editor/settings/ui/wireguardpeerwidget.ui
  libs/editor/settings/ui/wireguardtabwidget.ui
  libs/editor/settings/wireguardinterfacewidget.cpp
  libs/editor/settings/wireguardinterfacewidget.h
  libs/editor/settings/wireguardpeerwidget.cpp
  libs/editor/settings/wireguardpeerwidget.h
  libs/editor/settings/wireguardtabwidget.cpp
  libs/editor/settings/wireguardtabwidget.h
  libs/editor/wireguardkeyvalidator.cpp
  libs/editor/wireguardkeyvalidator.h
  libs/models/creatableconnectionsmodel.cpp
  libs/models/networkmodelitem.cpp
  libs/uiutils.cpp
  libs/uiutils.h
  vpn/CMakeLists.txt
  vpn/wireguard/CMakeLists.txt
  vpn/wireguard/Messages.sh
  vpn/wireguard/nm-wireguard-service.h
  vpn/wireguard/plasmanetworkmanagement_wireguardui.desktop
  vpn/wireguard/wireguard.cpp
  vpn/wireguard/wireguard.h
  vpn/wireguard/wireguard.ui
  vpn/wireguard/wireguardadvanced.ui
  vpn/wireguard/wireguardadvancedwidget.cpp
  vpn/wireguard/wireguardadvancedwidget.h
  vpn/wireguard/wireguardauth.cpp
  vpn/wireguard/wireguardauth.h
  vpn/wireguard/wireguardauth.ui
  vpn/wireguard/wireguardkeyvalidator.cpp
  vpn/wireguard/wireguardkeyvalidator.h
  vpn/wireguard/wireguardwidget.cpp
  vpn/wireguard/wireguardwidget.h

To: andersonbruce, jgrulich, #vdg, ngraham
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Jan Grulich]

jgrulich added inline comments.

INLINE COMMENTS

> kcm.cpp:527
> +
> +if (connection.isEmpty()) { // the "positive" part will arrive 
> with connectionAdded
> +// TODO display success

if (!connection.isEmpty()) {
  return;
  }

> connectionicon.cpp:403
>  }
> +} else if (type == 29) {  // TODO change to WireGuard enum 
> value once it is added
> +// WireGuard is a VPN but is not implemented

You can now use NetworkManager::Device::WireGuard.

> networkstatus.cpp:175
> +if (device && ((device->type() != 
> NetworkManager::Device::Generic && device->type() <= 
> NetworkManager::Device::Team)
> +   || device->type() == 29)) {  // TODO: Change to 
> WireGuard enum value when it is added
>  bool connecting = false;

You can use now NetworkManager::Device::WireGuard. It should be part of NMQT 
5.58 so we can use it for Plasma 5.16.

> wireguardinterfacewidget.cpp:620
> +}
> +if (!havePrivateKey || !haveAllowedIps || !havePublicKey || 
> !haveAllowedIps)
> +{

The "{" bracket should be on the same line with the "if"

> wireguardpeerwidget.cpp:52
> +
> +static WireGuardKeyValidator keyValidator(nullptr);
> +static SimpleIpListValidator allowedIPsValidator(nullptr, 
> SimpleIpListValidator::WithCidr,

You can change all the validators to take all the necessary parameters as first 
and then have a default value for the parent object so you don't need to pass a 
nullptr.

E.g.

  explicit SimpleIpV4AddressValidator(AddressStyle style = AddressStyle::Base, 
QObject *parent = nullptr);

I should have noticed this before. Can you change all the validators you use 
this way?

> wireguardpeerwidget.cpp:86
> +
> +WireGuardPeerWidget::WireGuardPeerWidget(const QVariantMap& peerData, 
> QWidget* parent, Qt::WindowFlags f)
> +: QDialog(parent, f)

WireGuardPeerWidget::WireGuardPeerWidget(const QVariantMap &peerData, QWidget 
*parent, Qt::WindowFlags f)

> wireguardpeerwidget.h:34
>  public:
> -explicit WireGuardAdvancedWidget(const NetworkManager::VpnSetting::Ptr 
> &setting, QWidget *parent = nullptr);
> -~WireGuardAdvancedWidget() override;
> -NetworkManager::VpnSetting::Ptr setting() const;
> +explicit WireGuardPeerWidget(const QVariantMap& peerData, QWidget* 
> parent = nullptr, Qt::WindowFlags f = {});
> +~WireGuardPeerWidget() override;

explicit WireGuardPeerWidget(const QVariantMap &peerData, QWidget *parent = 
nullptr, Qt::WindowFlags f = {});

> wireguardtabwidget.h:34
>  public:
> -explicit WireGuardAdvancedWidget(const NetworkManager::VpnSetting::Ptr 
> &setting, QWidget *parent = nullptr);
> -~WireGuardAdvancedWidget() override;
> -NetworkManager::VpnSetting::Ptr setting() const;
> +explicit WireGuardTabWidget(const NMVariantMapList& peerData, QWidget* 
> parent = nullptr, Qt::WindowFlags f = {});
> +~WireGuardTabWidget() override;

explicit WireGuardTabWidget(const NMVariantMapList &peerData, QWidget *parent = 
nullptr, Qt::WindowFlags f = {});

Sorry for being so pedantic :)

> wireguardtabwidget.h:39
> +
> +void loadConfig(const NMVariantMapList& peerData);
> +

void loadConfig(const NMVariantMapList &peerData);

> wireguardtabwidget.h:44
> +void slotAddPeer();
> +void slotAddPeerWithData(QVariantMap peerData);
> +void slotRemovePeer();

void slotAddPeerWithData(const QVariantMap &peerData);

> networkmodelitem.cpp:480
>  }
> -
> +// BAA Todo: add WireGuard
>  if (m_type == NetworkManager::ConnectionSettings::Wired) {

There is already WireGuard device in NMQT with KF5 5.58 which you should be 
able to use.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D20930

To: andersonbruce, jgrulich, #vdg, ngraham
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.


  Thanks for the info. I appreciate that you've taken so much time to think 
this through, and thanks for helping me understand how this works. LGTM from a 
#VDG  perspective!

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D20930

To: andersonbruce, jgrulich, #vdg, ngraham
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

andersonbruce updated this revision to Diff 57790.
andersonbruce added a comment.


  - Update labels on Add and Remove peer buttons

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20930?vs=57741&id=57790

BRANCH
  TwoTabs

REVISION DETAIL
  https://phabricator.kde.org/D20930

AFFECTED FILES
  applet/contents/ui/ConnectionItem.qml
  kcm/kcm.cpp
  kcm/qml/ConnectionItem.qml
  kded/notification.cpp
  kded/secretagent.cpp
  libs/declarative/connectionicon.cpp
  libs/declarative/networkstatus.cpp
  libs/declarative/networkstatus.h
  libs/editor/CMakeLists.txt
  libs/editor/connectioneditorbase.cpp
  libs/editor/settings/connectionwidget.cpp
  libs/editor/settings/ui/wireguardinterfacewidget.ui
  libs/editor/settings/ui/wireguardpeerwidget.ui
  libs/editor/settings/ui/wireguardtabwidget.ui
  libs/editor/settings/wireguardinterfacewidget.cpp
  libs/editor/settings/wireguardinterfacewidget.h
  libs/editor/settings/wireguardpeerwidget.cpp
  libs/editor/settings/wireguardpeerwidget.h
  libs/editor/settings/wireguardtabwidget.cpp
  libs/editor/settings/wireguardtabwidget.h
  libs/editor/wireguardkeyvalidator.cpp
  libs/editor/wireguardkeyvalidator.h
  libs/models/creatableconnectionsmodel.cpp
  libs/models/networkmodelitem.cpp
  libs/uiutils.cpp
  libs/uiutils.h
  vpn/CMakeLists.txt
  vpn/wireguard/CMakeLists.txt
  vpn/wireguard/Messages.sh
  vpn/wireguard/nm-wireguard-service.h
  vpn/wireguard/plasmanetworkmanagement_wireguardui.desktop
  vpn/wireguard/wireguard.cpp
  vpn/wireguard/wireguard.h
  vpn/wireguard/wireguard.ui
  vpn/wireguard/wireguardadvanced.ui
  vpn/wireguard/wireguardadvancedwidget.cpp
  vpn/wireguard/wireguardadvancedwidget.h
  vpn/wireguard/wireguardauth.cpp
  vpn/wireguard/wireguardauth.h
  vpn/wireguard/wireguardauth.ui
  vpn/wireguard/wireguardkeyvalidator.cpp
  vpn/wireguard/wireguardkeyvalidator.h
  vpn/wireguard/wireguardwidget.cpp
  vpn/wireguard/wireguardwidget.h

To: andersonbruce, jgrulich, #vdg
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

andersonbruce added a comment.


  In D20930#462775 , @ngraham wrote:
  
  > In D20930#462774 , 
@andersonbruce wrote:
  >
  > > Personally I like the Add and Remove buttons to be together where they 
were, and to simply change the wording to "Add New Peer" and "Remove This 
Peer". I think this still makes it clear that the peer in the tab currently 
being displayed in the dialog is the one that will be removed
  >
  >
  > All right, let's do that, then!
  
  
  I'll update to this.
  
  >> I haven't set up a large multi-user server but I imagine one could contain 
dozens if not hundreds of peers.
  > 
  > Hmm, if that's the case, then a tabbed UI may not actually be optimal. A 
list view, with a list of peers on the left side, a search above the list, and 
the peer information on the right, would be better if there may be enough peers 
to make access via tab view awkward. If so, I apologize for leading you down a 
bad path out of ignorance of the underlying technology. What do you think?
  
  I attempted to make a list view awhile ago and because of the length of some 
of the fields (in particular the key fields which are each 44 characters long) 
I couldn't come up with anything that looked decent. I think the tabbed 
interface will be fine in the vast majority of cases, especially in a desktop 
environment where typically there will only be one peer. Considering that the 
previous version only allowed one peer to be specified, this is a big 
improvement so I think at least for now and probably the foreseeable future, 
the tabbed interface is the way to go.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D20930

To: andersonbruce, jgrulich, #vdg
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Nathaniel Graham]

ngraham added a comment.


  In D20930#462774 , @andersonbruce 
wrote:
  
  > Personally I like the Add and Remove buttons to be together where they 
were, and to simply change the wording to "Add New Peer" and "Remove This 
Peer". I think this still makes it clear that the peer in the tab currently 
being displayed in the dialog is the one that will be removed
  
  
  All right, let's do that, then!
  
  > The wording should definitely not be "Connect to" because this form is used 
to edit a configuration not to actually make a connection.
  
  Gotcha, thanks for explaining!
  
  > I haven't set up a large multi-user server but I imagine one could contain 
dozens if not hundreds of peers.
  
  Hmm, if that's the case, then a tabbed UI may not actually be optimal. A list 
view, with a list of peers on the left side, a search above the list, and the 
peer information on the right, would be better if there may be enough peers to 
make access via tab view awkward. If so, I apologize for leading you down a bad 
path out of ignorance of the underlying technology. What do you think?

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D20930

To: andersonbruce, jgrulich, #vdg
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

andersonbruce added a comment.


  In D20930#462547 , @ngraham wrote:
  
  > So much better!
  >
  > I have one more little suggestion for the buttons: move the "Remove Peer" 
button into the bottom of the tab for that peer to make it clear what it 
applies to, and change the text to say, "Remove this Peer". The Remove button 
can stay where it is, but maybe should say, "Add New Peer". Actually, is "Add" 
the correct verb? Would "Connect to" be more appropriate?
  
  
  Here is a proposed version of what you asked for.  It also contains a little 
more context showing the parent dialog where the rest of the configuration is 
entered, behind the Peers dialog under discussion.
  
  Personally I like the Add and Remove buttons to be together where they were, 
and to simply change the wording to "Add New Peer" and "Remove This Peer". I 
think this still makes it clear that the peer in the tab currently being 
displayed in the dialog is the one that will be removed but am willing to do it 
the way shown below if the VDG considers it to be more consistent with the rest 
of KDE.
  
  The wording should definitely not be "Connect to" because this form is used 
to edit a configuration not to actually make a connection. Hitting "Connect" on 
a WireGuard configuration activates all the peers in that configuration at one 
time. I haven't set up a large multi-user server but I imagine one could 
contain dozens if not hundreds of peers.
  F6815862: RemoveBtnMoved.png 

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D20930

To: andersonbruce, jgrulich, #vdg
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Nathaniel Graham]

ngraham added a comment.


  So much better!
  
  I have one more little suggestion for the buttons: move the "Remove Peer" 
button into the bottom of the tab for that peer to make it clear what it 
applies to, and change the text to say, "Remove this Peer". The Remove button 
can stay where it is, but maybe should say, "Add New Peer". Actually, is "Add" 
the correct verb? Would "Connect to" be more appropriate?

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D20930

To: andersonbruce, jgrulich, #vdg
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

andersonbruce added a comment.


  In D20930#462365 , @ngraham wrote:
  
  > I'm afraid I don't have the ability to test this; could you post a new 
screenshot? Thanks!
  
  
  Here's a screenshot of the dialog with three peers:F6814829: Tabs.png 


REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D20930

To: andersonbruce, jgrulich, #vdg
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Nathaniel Graham]

ngraham added a comment.


  I'm afraid I don't have the ability to test this; could you post a new 
screenshot? Thanks!

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D20930

To: andersonbruce, jgrulich, #vdg
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

andersonbruce added a comment.


  A fairly large update was made to change the input of peer data to a use a 
QTabWidget interface to switch between peers rather than a spin box.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D20930

To: andersonbruce, jgrulich, #vdg
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

andersonbruce updated this revision to Diff 57741.
andersonbruce added a comment.


  Change to tab bar interface for peers

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20930?vs=57434&id=57741

BRANCH
  TwoTabs

REVISION DETAIL
  https://phabricator.kde.org/D20930

AFFECTED FILES
  applet/contents/ui/ConnectionItem.qml
  kcm/kcm.cpp
  kcm/qml/ConnectionItem.qml
  kded/notification.cpp
  kded/secretagent.cpp
  libs/declarative/connectionicon.cpp
  libs/declarative/networkstatus.cpp
  libs/declarative/networkstatus.h
  libs/editor/CMakeLists.txt
  libs/editor/connectioneditorbase.cpp
  libs/editor/settings/connectionwidget.cpp
  libs/editor/settings/ui/wireguardinterfacewidget.ui
  libs/editor/settings/ui/wireguardpeerwidget.ui
  libs/editor/settings/ui/wireguardtabwidget.ui
  libs/editor/settings/wireguardinterfacewidget.cpp
  libs/editor/settings/wireguardinterfacewidget.h
  libs/editor/settings/wireguardpeerwidget.cpp
  libs/editor/settings/wireguardpeerwidget.h
  libs/editor/settings/wireguardtabwidget.cpp
  libs/editor/settings/wireguardtabwidget.h
  libs/editor/wireguardkeyvalidator.cpp
  libs/editor/wireguardkeyvalidator.h
  libs/models/creatableconnectionsmodel.cpp
  libs/models/networkmodelitem.cpp
  libs/uiutils.cpp
  libs/uiutils.h
  vpn/CMakeLists.txt
  vpn/wireguard/CMakeLists.txt
  vpn/wireguard/Messages.sh
  vpn/wireguard/nm-wireguard-service.h
  vpn/wireguard/plasmanetworkmanagement_wireguardui.desktop
  vpn/wireguard/wireguard.cpp
  vpn/wireguard/wireguard.h
  vpn/wireguard/wireguard.ui
  vpn/wireguard/wireguardadvanced.ui
  vpn/wireguard/wireguardadvancedwidget.cpp
  vpn/wireguard/wireguardadvancedwidget.h
  vpn/wireguard/wireguardauth.cpp
  vpn/wireguard/wireguardauth.h
  vpn/wireguard/wireguardauth.ui
  vpn/wireguard/wireguardkeyvalidator.cpp
  vpn/wireguard/wireguardkeyvalidator.h
  vpn/wireguard/wireguardwidget.cpp
  vpn/wireguard/wireguardwidget.h

To: andersonbruce, jgrulich, #vdg
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Nathaniel Graham]

ngraham added a comment.


  Thanks!
  
  Seems like a tab bar would be the ideal UI here, with each peer having its 
own tab. That communicates the idea that the settings within the tab are 
specific to that tab/peer. Then the add and remove button should go below the 
tab bar's frame and say "Add peer" and "Remove peer" so it's clear what they 
refer to. Finally, the group box with the title "Peers" should be removed since 
the window's own title adequately communicates what you're looking at (and if 
it doesn't, that's what should be improved).
  
  Does that help?

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D20930

To: andersonbruce, jgrulich, #vdg
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

andersonbruce added a comment.


  In D20930#459505 , @ngraham wrote:
  
  > When #VDG  assistance is requested, 
screenshots of the current/proposed UI are appreciated. :)
  
  
  F6804483: PeersDialog.png 
  In the dialog shown, there can be several different "Peers" defined. The 
question involves the use of the SpinBox as the means of switching between the 
various entries. Each Peer has the same set of parameters to display/modify and 
changing the SpinBox value changes which one of them is being currently 
displayed. The "Add" and "Remove" buttons add a new Peer or delete an existing 
Peer and change the "of x peers" label next to the SpinBox to correspond to the 
current total number.
  
  I have not seen this exact method of changing a dialog before and while it 
seems intuitive to me, I don't know if it violates any KDE standards and I 
think both jgrulich and I just wanted to get input from the VDG before this is 
released.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D20930

To: andersonbruce, jgrulich, #vdg
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

andersonbruce marked 12 inline comments as done.
andersonbruce added a comment.


  Requested changes are made.

INLINE COMMENTS

> jgrulich wrote in connectionicon.cpp:403
> Do you want me to add this to NMQT?

Yes, please. This is part of the device type enum we discussed earlier where 
not all of the values defined in NM have been folded into NMQT yet.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D20930

To: andersonbruce, jgrulich, #vdg
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

andersonbruce updated this revision to Diff 57434.
andersonbruce marked an inline comment as done.
andersonbruce added a comment.


  - Correct review comment

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20930?vs=57433&id=57434

BRANCH
  TwoTabs

REVISION DETAIL
  https://phabricator.kde.org/D20930

AFFECTED FILES
  applet/contents/ui/ConnectionItem.qml
  kcm/kcm.cpp
  kcm/qml/ConnectionItem.qml
  kded/notification.cpp
  kded/secretagent.cpp
  libs/declarative/connectionicon.cpp
  libs/declarative/networkstatus.cpp
  libs/declarative/networkstatus.h
  libs/editor/CMakeLists.txt
  libs/editor/connectioneditorbase.cpp
  libs/editor/settings/connectionwidget.cpp
  libs/editor/settings/ui/wireguardinterfacewidget.ui
  libs/editor/settings/ui/wireguardpeerswidget.ui
  libs/editor/settings/wireguardinterfacewidget.cpp
  libs/editor/settings/wireguardinterfacewidget.h
  libs/editor/settings/wireguardpeerswidget.cpp
  libs/editor/settings/wireguardpeerswidget.h
  libs/editor/wireguardkeyvalidator.cpp
  libs/editor/wireguardkeyvalidator.h
  libs/models/creatableconnectionsmodel.cpp
  libs/models/networkmodelitem.cpp
  libs/uiutils.cpp
  libs/uiutils.h
  vpn/CMakeLists.txt
  vpn/wireguard/CMakeLists.txt
  vpn/wireguard/Messages.sh
  vpn/wireguard/nm-wireguard-service.h
  vpn/wireguard/plasmanetworkmanagement_wireguardui.desktop
  vpn/wireguard/wireguard.cpp
  vpn/wireguard/wireguard.h
  vpn/wireguard/wireguard.ui
  vpn/wireguard/wireguardadvanced.ui
  vpn/wireguard/wireguardadvancedwidget.cpp
  vpn/wireguard/wireguardadvancedwidget.h
  vpn/wireguard/wireguardauth.cpp
  vpn/wireguard/wireguardauth.h
  vpn/wireguard/wireguardauth.ui
  vpn/wireguard/wireguardkeyvalidator.cpp
  vpn/wireguard/wireguardkeyvalidator.h
  vpn/wireguard/wireguardwidget.cpp
  vpn/wireguard/wireguardwidget.h

To: andersonbruce, jgrulich, #vdg
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

andersonbruce updated this revision to Diff 57433.
andersonbruce added a comment.


  - Correct CamelCase of Wireguard to WireGuard
  - Disable adding WireGuard for NetworkManager < 1.16
  - Correct review comments

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20930?vs=57297&id=57433

BRANCH
  TwoTabs

REVISION DETAIL
  https://phabricator.kde.org/D20930

AFFECTED FILES
  applet/contents/ui/ConnectionItem.qml
  kcm/kcm.cpp
  kcm/qml/ConnectionItem.qml
  kded/notification.cpp
  kded/secretagent.cpp
  libs/declarative/connectionicon.cpp
  libs/declarative/networkstatus.cpp
  libs/declarative/networkstatus.h
  libs/editor/CMakeLists.txt
  libs/editor/connectioneditorbase.cpp
  libs/editor/settings/connectionwidget.cpp
  libs/editor/settings/ui/wireguardinterfacewidget.ui
  libs/editor/settings/ui/wireguardpeerswidget.ui
  libs/editor/settings/wireguardinterfacewidget.cpp
  libs/editor/settings/wireguardinterfacewidget.h
  libs/editor/settings/wireguardpeerswidget.cpp
  libs/editor/settings/wireguardpeerswidget.h
  libs/editor/wireguardkeyvalidator.cpp
  libs/editor/wireguardkeyvalidator.h
  libs/models/creatableconnectionsmodel.cpp
  libs/models/networkmodelitem.cpp
  libs/uiutils.cpp
  libs/uiutils.h
  vpn/CMakeLists.txt
  vpn/wireguard/CMakeLists.txt
  vpn/wireguard/Messages.sh
  vpn/wireguard/nm-wireguard-service.h
  vpn/wireguard/plasmanetworkmanagement_wireguardui.desktop
  vpn/wireguard/wireguard.cpp
  vpn/wireguard/wireguard.h
  vpn/wireguard/wireguard.ui
  vpn/wireguard/wireguardadvanced.ui
  vpn/wireguard/wireguardadvancedwidget.cpp
  vpn/wireguard/wireguardadvancedwidget.h
  vpn/wireguard/wireguardauth.cpp
  vpn/wireguard/wireguardauth.h
  vpn/wireguard/wireguardauth.ui
  vpn/wireguard/wireguardkeyvalidator.cpp
  vpn/wireguard/wireguardkeyvalidator.h
  vpn/wireguard/wireguardwidget.cpp
  vpn/wireguard/wireguardwidget.h

To: andersonbruce, jgrulich, #vdg
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Nathaniel Graham]

ngraham added a reviewer: VDG.
ngraham added a comment.


  When #VDG  assistance is requested, 
screenshots of the current/proposed UI are appreciated. :)

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D20930

To: andersonbruce, jgrulich, #vdg
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Jan Grulich]

jgrulich added a subscriber: ngraham.
jgrulich added a comment.


  Can we maybe have some assistence from anyone from VDG? @ngraham maybe?

INLINE COMMENTS

> CMakeLists.txt:11
>  
> +set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -g")
> +

Please remove.

> kcm.cpp:264
>  NetworkManager::ConnectionSettings::ConnectionType type = 
> static_cast(connectionType);
> -
> +qCWarning(PLASMA_NM) << "onRequestCreateConnection type: " << type;
>  if (type == NetworkManager::ConnectionSettings::Vpn && vpnType == 
> "imported") {

Please remove.

> connectionicon.cpp:403
>  }
> +} else if (type == 29) {  // TODO change to WireGuard enum 
> value once it is added
> +// WireGuard is a VPN but is not implemented

Do you want me to add this to NMQT?

> connectioneditorbase.cpp:151
>  }
> -
> +#if 1
> +qCWarning(PLASMA_NM) << "ConnectionEditorBase: Before toMap";

Please remove

> connectioneditorbase.cpp:198
>  
> +qCWarning(PLASMA_NM) << "ConnectionEditorBase:" << m_connection->id();
> +

Please remove.

> connectioneditorbase.cpp:264
>  if (!vpnSetting) {
> -qCWarning(PLASMA_NM) << "Missing VPN setting!";
> +qCWarning(PLASMA_NM) << "Missing VPN setting 2!";
>  } else {

Why the "2" at the end?

> connectioneditorbase.cpp:458
>  for (const QString &key : secrets.keys()) {
> +qCWarning(PLASMA_NM) << "replyFinished key: " << key << "  
> settingName: " << settingName;
>  if (key == settingName) {

Please remove.

> connectioneditorbase.cpp:465
>  const QString type = widget->type();
> +qCWarning(PLASMA_NM) << "type: " << type << "   settingName: " << 
> settingName;
>  if (type == settingName ||

Please remove.

> wireguardinterfacewidget.cpp:114
> +WireGuardInterfaceWidget::WireGuardInterfaceWidget(const 
> NetworkManager::Setting::Ptr &setting, QWidget* parent, Qt::WindowFlags f):
> +SettingWidget(setting, parent, f),
> +d(new Private)

Coding style.

Should be:

  WireGuardInterfaceWidget::WireGuardInterfaceWidget(const 
NetworkManager::Setting::Ptr &setting, QWidget* parent, Qt::WindowFlags f)
  : SettingWidget(setting, parent, f)
  , d(new Private)

> wireguardinterfacewidget.cpp:182
> +
> +if (0 != wireGuardSetting->listenPort())
> +
> d->ui.listenPortLineEdit->setText(QString::number(wireGuardSetting->listenPort()));

I would preffer:

  if (wireGuardSetting->listenPort() != 0)

It's more readable. Same for code below.

> wireguardinterfacewidget.cpp:250
> +{
> +
> +NetworkManager::WireGuardSetting wgSetting;

Remove empty line

> wireguardinterfacewidget.cpp:424
> +
> +else if (keyValue[0] == PNM_WG_CONF_TAG_PEER) {
> +// Check to make sure the previous PEER section has

Coding style. The "else if" should be on the same line as the "}" bracket.

> wireguardinterfacewidget.cpp:482
> +// Listen Port
> +else if (key == PNM_WG_CONF_TAG_LISTEN_PORT) {
> +uint val = keyValue[1].toUInt();

Same here and below.

> wireguardinterfacewidget.cpp:568
> +else if (currentState == PEER_SECTION)
> +{
> +// Public Key

The "{" bracket should be on the same line as the "else if". Same below.

> wireguardinterfacewidget.cpp:633
> +peers.append(*currentPeer);
> +#endif
> +#if 1

Please remove

> wireguardpeerswidget.cpp:82
> +
> +WireGuardPeersWidget::WireGuardPeersWidget(const NMVariantMapList& peerData, 
> QWidget* parent, Qt::WindowFlags f):
> +QDialog(parent, f),

Coding style.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D20930

To: andersonbruce, jgrulich
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20930: Update WireGuard to match NetworkManager 1.16 interface

[Bruce Anderson]

andersonbruce created this revision.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
Herald added a reviewer: jgrulich.
andersonbruce requested review of this revision.

REVISION SUMMARY
  In NetworkManager 1.16 handling of WireGuard interfaces was changed
  from a VPN add-on to a core interface type with a different API. This
  plasma-nm update is intended to match that change including (but not
  limited to) moving address specification to the IPv4 and IPv6 tabs and
  the ability to add multiple Peers to an interface.
  
  BUG: 405501

REPOSITORY
  R116 Plasma Network Management Applet

BRANCH
  TwoTabs

REVISION DETAIL
  https://phabricator.kde.org/D20930

AFFECTED FILES
  CMakeLists.txt
  applet/contents/ui/ConnectionItem.qml
  kcm/kcm.cpp
  kcm/qml/ConnectionItem.qml
  kded/notification.cpp
  kded/secretagent.cpp
  libs/declarative/connectionicon.cpp
  libs/declarative/networkstatus.cpp
  libs/declarative/networkstatus.h
  libs/editor/CMakeLists.txt
  libs/editor/connectioneditorbase.cpp
  libs/editor/settings/connectionwidget.cpp
  libs/editor/settings/ui/wireguardinterfacewidget.ui
  libs/editor/settings/ui/wireguardpeerswidget.ui
  libs/editor/settings/wireguardinterfacewidget.cpp
  libs/editor/settings/wireguardinterfacewidget.h
  libs/editor/settings/wireguardpeerswidget.cpp
  libs/editor/settings/wireguardpeerswidget.h
  libs/editor/wireguardkeyvalidator.cpp
  libs/editor/wireguardkeyvalidator.h
  libs/models/creatableconnectionsmodel.cpp
  libs/models/networkmodelitem.cpp
  libs/uiutils.cpp
  libs/uiutils.h
  vpn/CMakeLists.txt
  vpn/wireguard/CMakeLists.txt
  vpn/wireguard/Messages.sh
  vpn/wireguard/nm-wireguard-service.h
  vpn/wireguard/plasmanetworkmanagement_wireguardui.desktop
  vpn/wireguard/wireguard.cpp
  vpn/wireguard/wireguard.h
  vpn/wireguard/wireguard.ui
  vpn/wireguard/wireguardadvanced.ui
  vpn/wireguard/wireguardadvancedwidget.cpp
  vpn/wireguard/wireguardadvancedwidget.h
  vpn/wireguard/wireguardauth.cpp
  vpn/wireguard/wireguardauth.h
  vpn/wireguard/wireguardauth.ui
  vpn/wireguard/wireguardkeyvalidator.cpp
  vpn/wireguard/wireguardkeyvalidator.h
  vpn/wireguard/wireguardwidget.cpp
  vpn/wireguard/wireguardwidget.h

To: andersonbruce, jgrulich
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart