Add WireGuard capability.
ClosedPublic

Authored by andersonbruce on Aug 27 2018, 4:05 AM.

Details

Summary

FEATURE: 397572
FIXED-IN: 5.14.0

Diff Detail

Repository
R116 Plasma Network Management Applet
Branch
Feature/AddWireGuardVPN
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 3109
Build 3127: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
pino added inline comments.Sep 12 2018, 7:08 AM
libs/editor/simpleipv6addressvalidator.cpp
50–57 ↗(On Diff #41451)

as above, created every time, and leaked

This revision now requires changes to proceed.Sep 12 2018, 7:08 AM
  • Add tests for new and updated ip validators
  • Update ip validators
  • Add validator for WireGuard keys
  • Update per review comments
  • Add error messages for failure cases
andersonbruce marked 45 inline comments as done.Sep 16 2018, 7:59 AM
andersonbruce added inline comments.
vpn/wireguard/wireguard.cpp
122

Used plain 'for' instead because Nathaniel Graham commented that the Q_FOREACH is not acceptable anymore.

Thank you @andersonbruce for all of your work on this, and quickly working on feedback changes! This will be a great addition.

andersonbruce marked an inline comment as done.Sep 17 2018, 5:43 AM
andersonbruce added inline comments.
vpn/wireguard/wireguardadvancedwidget.cpp
105

Functionally I think that the current implementation does this (although I can change the name of the function if you want). It starts with a blank NMStringMap and uses setOrClear on it. Are you possibly referring to the same function name used in wireguardwidget.cpp rather than here in wireguardadvancedwidget.cpp?

Please, update all your reviews so they don't duplicate changes. I would personally have one review for all your IPv[4,6]Validator changes and one review just for WireGuard VPN plugin.

andersonbruce marked an inline comment as done.
  • Remove changes moved to review D15520

Since I added a validator function for the WireGuard style keys, is there any way to assign a validator to the PasswordField widget without a fairly substantial rewrite of that class?

pino requested changes to this revision.Sep 18 2018, 5:41 AM

note there are still few "not done" comments around (eg using QSpinBox for fwMark)

CMakeLists.txt
28 ↗(On Diff #41874)
vpn/wireguard/wireguard.cpp
140

no need to specify this as parent, since it is on stack, and thus it will be deleted automatically

140–141

indent

178

since earlier there is a keyPos variable used for basically the same purpose (i.e. passing it to the validate() of validators), just move pos earlier in the function, and use one everywhere

vpn/wireguard/wireguardadvancedwidget.cpp
27

unused

32

unused

121

a bit confusing to call intVal a variable which is (not correctly) a long; OTOH, it can be removed altogether (see below)

122

unused

124–128

just inline the calls to value(), just like done below

vpn/wireguard/wireguardadvancedwidget.h
28

unused

vpn/wireguard/wireguardkeyvalidator.h
27

no need for Q_DECL_EXPORT, as it is in a plugin, and thus it does not need to be exported as public symbol

vpn/wireguard/wireguardwidget.cpp
183

this is not "done", btw

This revision now requires changes to proceed.Sep 18 2018, 5:41 AM

Since I added a validator function for the WireGuard style keys, is there any way to assign a validator to the PasswordField widget without a fairly substantial rewrite of that class?

We don't do validation inside the passwordfield widget, this is done outside in widgets using it, you should do the same. Given it's a QWidget, you cannot directly assign a validator to it, but inside is QLineEdit which can have validator, you would have add a method to the main class which would just assign the validator to the QLineEdit widget inside, still please do the validation outside.

In D15093#327917, @pino wrote:

note there are still few "not done" comments around (eg using QSpinBox for fwMark)

Yes I know, I'll get to the spinbox in the next day or so. I've only been putting it off because I think it is such a bad idea and I hate making something I wrote worse rather than better but I know that I am playing in your house so I need to play by your rules. I'll hold my nose and get to it soon.

pino added a comment.Sep 18 2018, 9:33 PM
In D15093#327917, @pino wrote:

note there are still few "not done" comments around (eg using QSpinBox for fwMark)

Yes I know, I'll get to the spinbox in the next day or so. I've only been putting it off because I think it is such a bad idea and I hate making something I wrote worse rather than better but I know that I am playing in your house so I need to play by your rules. I'll hold my nose and get to it soon.

What you say sounds like you are understanding my notes as "imposing on you" "bad choices", which is definitely not the case. What I note are things based on my experience, and what I read from the patch.
If I see a configuration key which is validated as positive integer, then my conclusion is that it makes sense to use the proper widget for it, i.e. QSpinBox (with proper range). If you think it is not correct, please do explain it! I could be missing anything, or simply knowing better about it is a good idea regardless.

But please do not play the card of "you are the bad guy imposing things on me", thanks.

In D15093#328246, @pino wrote:
In D15093#327917, @pino wrote:

note there are still few "not done" comments around (eg using QSpinBox for fwMark)

Yes I know, I'll get to the spinbox in the next day or so. I've only been putting it off because I think it is such a bad idea and I hate making something I wrote worse rather than better but I know that I am playing in your house so I need to play by your rules. I'll hold my nose and get to it soon.

What you say sounds like you are understanding my notes as "imposing on you" "bad choices", which is definitely not the case. What I note are things based on my experience, and what I read from the patch.
If I see a configuration key which is validated as positive integer, then my conclusion is that it makes sense to use the proper widget for it, i.e. QSpinBox (with proper range). If you think it is not correct, please do explain it! I could be missing anything, or simply knowing better about it is a good idea regardless.

But please do not play the card of "you are the bad guy imposing things on me", thanks.

I'm sorry, I am not trying to paint you as a bad guy here. What I meant to say is that I disagree with you that using a QSpinBox in these cases is a good idea. I just realize that different projects have different design philosophies / standards and that since I am working on your project which I am a newcomer to, that I am going to follow your recommendations even if they disagree with my personal design philosophy because I realize it is important to be consistent across multiple projects of which this is only a tiny piece of just one project.

In the case of the spinbox, I personally don't think that they should be used for anything with more than about 100 different steps because, by their nature, they tell the user to use the up and down arrows to change the value. Now you know and I know that you can just type in a value but not everyone does and on a value that can range from 0 to a very large number (in this case 2000000000+) I believe typing in a value makes sense 99.99% of the time and that using a text box tells the user: "Type a number in here" better than a spinbox does. The same is true to a slightly lesser extent for the other two items which have been changed to spinboxes. Also since all of these will almost always not be used, again my personal opinion is that a blank text box conveys that better than sticking a "default" or "automatic" down at the low end of the spinbox range again because not everyone is going to know to type in 0 to get back to "default" if somehow it gets changed.

It was late at night when your message about not forgetting about the spinbox (which I hadn't forgotten) came in and it sounded at the time a little like someone saying "I've told you this before, now you have to do this". Obviously this was not your intention and I apologize that I let my frustration get to me a little.

Perhaps mistakenly, I have taken most of the 'design' related comments (as opposed to the 'implementation' issues) as being "this is the KDE way of doing things" and since that is a perfectly valid reason for doing things a particular way, I've just said "Okay, I'll do this even though I disagree that it is a good idea" and got a little frustrated about them. Maybe I should have pushed back a little harder on this and other issues I disagreed with. Who knows. It's all water under the bridge now. Again, my apologies.

ngraham added a comment.EditedSep 19 2018, 3:45 AM

We generally try to operate on the principle of consensus here in the KDE community, so if there's a disagreement, we talk about it until someone changes their mind or we arrive at a compromise.

FWIW, I would tend to agree that using a spinbox doesn't make sense when the range is enormous and there isn't much of a chance that it would ever make sense to use the arrows to adjust the value.

andersonbruce marked an inline comment as done.
  • Update from more (but not all) review comments
  • Change 'setting' functions to use blank NMStringMap rather than incoming data.

Okay, I just uploaded changes for most of the rest of the comments but I would like to revisit a couple of earlier issues.
The first is the SpinBoxes. As I said in another comment, I don't think that the SpinBoxes are appropriate for any of the entries and would like to return these to be plain LineEdit widgets. The smallest range for any of these is the port number box at 0-65535 and I don't think any of them will be entered using the SpinBox arrows so I think simple text entry is better from an HMI perspective.

Secondly I would like to reconsider entry validation. Currently it is using Pino's suggestion of using Validators on each entry to not allow incorrect keys to be entered and I think this is a good approach but I don't think it is sufficient. Right now there are 5 items checked for validity on the main screen (plus a couple on the Advanced settings widget) which can be checked for validity and when they are all valid, the "Save" button is enabled.

These are obviously different in that one tests to see if the entry is completely valid while the other only checks to see if it can still be made to be complete and valid. My problem is that if there are five things that need to be checked and only one bit of information (whether the Save button is enabled or disabled) to indicate to the user that there is a problem and no indication of which element might be in error.

I would like to add some type of indicator on each entry line to indicate whether that particular line is valid so if the user has entered: "BBBB:0:1223:" for an IPv6 address (which is not valid) and all the other lines are valid, the user would be able to see immediately which line they need to look at to go forward. This was the idea behind my initial code which changed the background color of widgets to indicate validity. I realize that there are problems with this approach so I would like to come up with some other means to supply this information to the user. I've done three mockups of some possibilities:


The difference between the last two is whether the CheckBoxes are enabled or not. I think that the enabled CheckBoxes is the best looking although it requires a little more programming because since it would be utilized as a strictly output widget I would have to intercept any clicks on the check box and ensure that it stayed in the correct state.

Any comments would be appreciated.

How about changing the outline or background of the text box to the Positive or Negative color from the active color theme? That way (for people with sensible color themes), the text box would show red if the value is invalid. I believe GNOME does something similar to this for their IP address validator, and in my experience it works quite nicely.

I agree with @ngraham, I was also planning a similar approach for other connection types.

  • Add entry widget background color change based on entry validity
  • Change QSpinBoxes back to QLineEdits
  • Add entry widget background color change based on entry validity
  • Change QSpinBoxes back to QLineEdits

Awesome, +1 from me on these changes!

andersonbruce marked 13 inline comments as done.Sep 26 2018, 4:02 AM

I've uploaded everything I wanted to get done now. Are there any new comments? And by the way, what is the process once the all the comments do get cleared?

Well for the first time since July there has been activity on network-manager-wireguard including the addition of an additional parameter supported so I am going to get going on adding it to this. Also, it looks like the pre- and post- options now do something so against my better judgment I am going to add them back in as well.

jgrulich added inline comments.Oct 17 2018, 11:44 AM
vpn/wireguard/CMakeLists.txt
9

Missing wireguardkeyvalidator.cpp

30

Linking against following ones is enough:

plasmanm_internal
plasmanm_editor
KF5::ConfigCore
KF5::CoreAddons
KF5::I18n
KF5::KIOWidgets
KF5::WidgetsAddons
vpn/wireguard/plasmanetworkmanagement_wireguardui.desktop
17

Name could be just WireGuard I guess

vpn/wireguard/wireguardadvancedwidget.cpp
42

Coding style

59

Coding style

64

Coding style

88

This validation doesn't seem to work, I can click on "ok" button even when the advanced widget is empty. Also it's a bit weird that the function name suggests something else then it's doing. I would suggest splitting this into something like "updatePresharedKeyValidation()" and to the one you have now. One would really do just the check, the other would update the color.

Same please do for classic wireguard widget.

93

Perhaps you should write a destructor for the private widget and here just delete the d-pointer.

vpn/wireguard/wireguardadvancedwidget.h
43

Coding style

vpn/wireguard/wireguardwidget.cpp
31

KColorScheme has been removed/deprecated in KDE Frameworks and it's only in KDELibs4support which we don't want to use. You should use QPalette instead.

40

You also will not need this, because QPalette already should have used colors thanks to our KDE platform theme.

61

Do not use KColorScheme. Use QPalette instead and you don't need to keeping it as class variable.

74

Split isFooValid() functions, as mentioned in the advanced widget. Also slotWidgetChanged() will call isValid() I think, which means that each isFooValid() is called twice.

197

Coding style

andersonbruce added inline comments.Oct 18 2018, 10:20 AM
vpn/wireguard/wireguardwidget.cpp
61

My apologies if I sound a little frustrated on this but I spent more than 4 hours trying to follow the very unclear documentation on handling colors before making this change. I thought the whole idea was that I was supposed to use the scheme the user picked to tell what color to make the background when it wasn't valid rather than assigning an arbitrary color. KColorScheme allows you to do this using the "NegativeBackground" role but there doesn't appear to be any corresponding concept using QPalette. What would you suggest that I use as the background for invalid entries? Or better yet. do you know of an example of something that uses QPalette in a similar context since I have been unable to find one?

As far as putting it in a class variable, I only did that so that I wouldn't be creating them each time I wanted to change a background on one of the widgets and to be able to pass them to the advanced widget and not have to create them there as well. If you think that palette creation is a relatively time efficient process, I'll just create them on the stack in the setBackground() function each time they are needed.

jgrulich added inline comments.Oct 18 2018, 10:41 AM
vpn/wireguard/wireguardwidget.cpp
61

I'm sorry, I was wrong and I thought that KColorPalette is deprecated, but it's actually not and it's part of KConfigWidgets. Still, given you just use those two colors, you don't need to save full palette, just construct a one to get the colors and save these as member variables. You can do basically same for the advanced widget, you don't need to pass them in constructor.

  • Address review comments

A couple of notes on the latest upload:

  1. I left the background color change code alone. Doing searches on changing widget background color, it looks like the preferred method is to use setPalette() which requires a QPalette, and after trying a couple of things I decided it made more sense to me to pass around two palettes by reference than to pass just the colors and create palettes when they were needed.
  1. I backed out the change that used a blank setting rather than the stored values. I agree that it would be nice to not hang on to any extraneous elements if they happened to be created by some other application but what I found was that I had somehow not tested it after making this change and when I did, I found that it had broken the entire "Advanced" widget settings. The problem is that if you start with a blank setting and don't open the Advanced Widget then all the settings in that are lost. To fix this I would have to duplicate a bunch of functionality in both widgets which I don't like doing. Also, if the underlying NetworkManager app adds something I think that it is probably better to not delete it if plasma-nm hasn't been updated yet so I ended up deciding to put it back to the way it was.
andersonbruce marked 9 inline comments as done.Oct 25 2018, 6:15 AM
andersonbruce added inline comments.
vpn/wireguard/plasmanetworkmanagement_wireguardui.desktop
17

I left this alone because it is consistent with the names of other VPNs.

vpn/wireguard/wireguardadvancedwidget.cpp
88

I did this slightly different from the suggestion but it only does the checks once per change and I think it is correctly enabling/disabling the "Ok" button. By the way, the "Ok" button is enabled when all entries on the Advanced widget are blank because all of the entries are optional and therefore all blank is perfectly valid.

I still don't like the way how to get QPalette in the advanced dialog, can you please just simply construct it the same way you do it in the standard dialog? Other than that it looks good and I think it's ready to go. Those mentioned coding style can be fixed later, I can go through that after it's merged.

vpn/wireguard/wireguardadvancedwidget.cpp
49

Coding style, but can be fixed afterwards.

vpn/wireguard/wireguardwidget.cpp
53

Coding style, can be fixed afterwards.

76

Coding style

andersonbruce marked an inline comment as done.
  • Allocate palettes in advanced widget rather than passing them from main interface

I still don't like the way how to get QPalette in the advanced dialog, can you please just simply construct it the same way you do it in the standard dialog?

Fair enough. I changed it so the palettes are created in the advanced widget as well as the main interface.
I fixed one of the coding style errors which I just missed last time. The other ones I will let you fix because I'm not sure what the correct format would be.

Thanks for your patience with all my mistakes in this process.

jgrulich accepted this revision.Nov 7 2018, 8:33 AM
This revision was not accepted when it landed; it landed in state Needs Review.Nov 7 2018, 8:53 AM
Closed by commit R116:389a5e195bce: Add WireGuard capability. (authored by andersonbruce, committed by jgrulich). · Explain Why
This revision was automatically updated to reflect the committed changes.

Very cool! Does this actually implement full native WireGuard support, or only add support for the NetworkManager plugin available at https://github.com/max-moser/network-manager-wireguard?

Very cool! Does this actually implement full native WireGuard support, or only add support for the NetworkManager plugin available at https://github.com/max-moser/network-manager-wireguard?

It only provides a front end for the NetworkManager plugin. This keeps the network handling all in NetworkManager but allows setup and display to be integrated into the KDE/Plasma applet.

Ah, thanks. And presumably shipping that networkmanager plugin would be up to distros, right?

Ah, thanks. And presumably shipping that networkmanager plugin would be up to distros, right?

I assume this is the case.

There are several NetworkManager VPN plugins that are handled this way where the code is not part of the official release but are referenced on the Gnome homepage for the project as being supported by third parties. Most if not all of these are already supported in the plasma-nm release and this just adds WireGuard to that group. I know that at least openSuse Tumbleweed supplies some of these third party plugins (although not WireGuard yet) so hopefully as WireGuard becomes more popular it will join this group as well.

Thanks for the explanation! Just what I needed to feature this in https://pointieststick.wordpress.com/2018/11/07/this-week-in-usability-productivity-part-44 (link will work starting this Sunday).