D27764: Update L2TP to NetworkManager-l2tp 1.8.0 features

[Jan Grulich]

This revision was automatically updated to reflect the committed changes.
Closed by commit R116:999f0ad9fb8b: Update L2TP  to NetworkManager-l2tp 1.8.0 
features (authored by jgrulich).

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27764?vs=76933=76989

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

AFFECTED FILES
  vpn/l2tp/CMakeLists.txt
  vpn/l2tp/l2tp.cpp
  vpn/l2tp/l2tp.ui
  vpn/l2tp/l2tpauth.cpp
  vpn/l2tp/l2tpauth.h
  vpn/l2tp/l2tpauth.ui
  vpn/l2tp/l2tpipsec.ui
  vpn/l2tp/l2tpipsecwidget.cpp
  vpn/l2tp/l2tpipsecwidget.h
  vpn/l2tp/l2twidget.cpp
  vpn/l2tp/l2twidget.h
  vpn/l2tp/l2tpwidget.cpp
  vpn/l2tp/l2tpwidget.h
  vpn/l2tp/nm-l2tp-service.h

To: dkosovic, jgrulich
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27764: Update L2TP to NetworkManager-l2tp 1.8.0 features

[Jan Grulich]

jgrulich added a comment.


  I would say it's not a big deal as we use groupboxes quite a lot in 
plasma-nm. We can get rid of them in future when rewriting the KCM to QML.

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

To: dkosovic, jgrulich
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27764: Update L2TP to NetworkManager-l2tp 1.8.0 features

[Nathaniel Graham]

ngraham added a comment.


  It's just that in general we're trying to move away from group boxes and 
towards the use of whitespace to logically group sections. It's not a huge 
deal, but it would be appreciated. :)

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

To: dkosovic, jgrulich
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27764: Update L2TP to NetworkManager-l2tp 1.8.0 features

[Douglas Kosovic]

dkosovic added a comment.


  When I was updating this client's UI, I was pretty conflicted about using 
group boxes, many of the other VPN clients were using group boxes for their 
main window and at one stage I was thinking of introducing group boxes for the 
main window of this client. It was definitely this client's PPP settings dialog 
which uses multiple group boxes that initially convinced me to use group boxes 
with the IPsec settings dialog.
  
  I don't know what is best for the UI.

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

To: dkosovic, jgrulich
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27764: Update L2TP to NetworkManager-l2tp 1.8.0 features

[Douglas Kosovic]

dkosovic updated this revision to Diff 76933.
dkosovic added a comment.


  updates based on review comments

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27764?vs=76731=76933

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

AFFECTED FILES
  vpn/l2tp/CMakeLists.txt
  vpn/l2tp/l2tp.cpp
  vpn/l2tp/l2tp.ui
  vpn/l2tp/l2tpauth.cpp
  vpn/l2tp/l2tpauth.h
  vpn/l2tp/l2tpauth.ui
  vpn/l2tp/l2tpipsec.ui
  vpn/l2tp/l2tpipsecwidget.cpp
  vpn/l2tp/l2tpipsecwidget.h
  vpn/l2tp/l2twidget.cpp
  vpn/l2tp/l2twidget.h
  vpn/l2tp/l2tpwidget.cpp
  vpn/l2tp/l2tpwidget.h
  vpn/l2tp/nm-l2tp-service.h

To: dkosovic, jgrulich
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27764: Update L2TP to NetworkManager-l2tp 1.8.0 features

[Douglas Kosovic]

dkosovic added a comment.


  I'm of mixed minds on the group boxes.
  
  With L2TP/IPsec connection setup instructions for macOS and iOS, they 
typically have screenshots of the "User Authentication" and "Machine 
Authentication" settings. Many Linux users try to match what they see for other 
platforms to what they need to enter. A typical user probably wouldn't know 
that machine authentication is IPsec authentication, although if it is a 
pre-shared key it doesn't need to be spelt out, for certificates it can get a 
bit more confusing between user and machine certificates. But having said that, 
I did explicitly use "User Certificate" and "Machine Certificate" labels, 
perhaps most users would realize "Machine Certificate" corresponds to "Machine 
Authentication"?
  
  I got the idea of using a checkable group box as the existing PPP settings 
dialog used one, but granted it didn't have nested group boxes. I was trying to 
imply with the Advanced group box that it is optional and should only be 
touched if you know what you are doing.

REPOSITORY
  R116 Plasma Network Management Applet

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

To: dkosovic, jgrulich
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27764: Update L2TP to NetworkManager-l2tp 1.8.0 features

[Nathaniel Graham]

ngraham added a comment.


  -1 for using group boxes. :) I don't think they're needed at all. Just change 
the "Type:" label to "Authentication:" and that section is clear enough, then 
just separate the logical sections with whitespace

REPOSITORY
  R116 Plasma Network Management Applet

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

To: dkosovic, jgrulich
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27764: Update L2TP to NetworkManager-l2tp 1.8.0 features

[Jan Grulich]

jgrulich added a comment.


  In D27764#621204 , @dkosovic wrote:
  
  > In D27764#621173 , @jgrulich 
wrote:
  >
  > > @dkosovic will you update the review to address my comments?
  >
  >
  > I agree with all your comments and they are great nitpicks and suggestions.
  >
  > Sorry I didn't have time tonight to work on testing everything and 
submitting an updated patch. I'm guessing submitting an updated patch is what I 
need to do for the review?
  
  
  No problem, take your time. I don't know the way you submitted this change, 
whether you used **arc** to do so or you just uploaded a patch. With **arc** 
you just create another commit and run again:
  
arc diff
  
  If you uploaded just a patch, then I guess you will need to update it the 
same way.

REPOSITORY
  R116 Plasma Network Management Applet

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

To: dkosovic, jgrulich
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27764: Update L2TP to NetworkManager-l2tp 1.8.0 features

[Douglas Kosovic]

dkosovic added a comment.


  In D27764#621173 , @jgrulich wrote:
  
  > @dkosovic will you update the review to address my comments?
  
  
  I agree with all your comments and they are great nitpicks and suggestions.
  
  Sorry I didn't have time tonight to work on testing everything and submitting 
an updated patch. I'm guessing submitting an updated patch is what I need to do 
for the review?

REPOSITORY
  R116 Plasma Network Management Applet

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

To: dkosovic, jgrulich
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27764: Update L2TP to NetworkManager-l2tp 1.8.0 features

[Jan Grulich]

jgrulich added a comment.


  @dkosovic will you update the review to address my comments?

REPOSITORY
  R116 Plasma Network Management Applet

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

To: dkosovic, jgrulich
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27764: Update L2TP to NetworkManager-l2tp 1.8.0 features

[Jan Grulich]

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


  Only nitpicking, otherwise it looks good to me.

INLINE COMMENTS

> l2tpauth.cpp:34
> +#include "nm-l2tp-service.h"
> +#include "debug.h"
> +

This include doesn't seem to be used.

> l2tpipsecwidget.cpp:127
> +if (!dataMap[NM_L2TP_KEY_IPSEC_IKELIFETIME].isEmpty()) {
> +int totalSeconds = 
> dataMap[NM_L2TP_KEY_IPSEC_IKELIFETIME].toInt();
> +int hours = totalSeconds / 3600;

const? Same for variables below.

> l2tpipsecwidget.cpp:133
> +
> + m_ui->ikelifetime->setTime(lifetime);
> +m_ui->ikelifetime->setEnabled(true);

Missing indentation.

> l2tpipsecwidget.cpp:143
> +if (!dataMap[NM_L2TP_KEY_IPSEC_SALIFETIME].isEmpty()) {
> +int totalSeconds = dataMap[NM_L2TP_KEY_IPSEC_SALIFETIME].toInt();
> +int hours = totalSeconds / 3600;

const? Same for variables below.

> l2tpipsecwidget.cpp:170
> +if (dataMap[NM_L2TP_KEY_IPSEC_PFS] == noString ) {
> +m_ui->cbPFS->setChecked(true);
> +} else {

This can be simplified using:

  m_ui->cbIPComp->setChecked(dataMap[NM_L2TP_KEY_IPSEC_IPCOMP] == yesString)

Same above.

> l2tpipsecwidget.cpp:259
> +if (m_ui->cbIkelifetime->isChecked()) {
> +int totalSeconds = m_ui->ikelifetime->time().hour() * 3600 + 
> m_ui->ikelifetime->time().minute() * 60 + m_ui->ikelifetime->time().second();
> +

const?

> l2tpipsecwidget.cpp:265
> +if (m_ui->cbSalifetime->isChecked()) {
> +int totalSeconds = m_ui->salifetime->time().hour() * 3600 + 
> m_ui->salifetime->time().minute() * 60 + m_ui->salifetime->time().second();
> +result.insert(NM_L2TP_KEY_IPSEC_SALIFETIME, 
> QString::number(totalSeconds));

const?

> l2tpipsecwidget.cpp:304
> +requesters << m_ui->machineCA << m_ui->machineCert << m_ui->machineKey;
> +bool isP12 = url.toString().endsWith(QLatin1String(".p12"));
> +

const?

> l2twidget.cpp:60
> +// Authentication options
> +bool refuse_pap = (dataMap[NM_L2TP_KEY_REFUSE_PAP] == yesString);
> +bool refuse_chap = (dataMap[NM_L2TP_KEY_REFUSE_CHAP] == yesString);

const? Same for variables below.

REPOSITORY
  R116 Plasma Network Management Applet

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

To: dkosovic, jgrulich
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27764: Update L2TP to NetworkManager-l2tp 1.8.0 features

[Douglas Kosovic]

dkosovic added a comment.


  This patch is depends on D27763  being 
applied first.
  
  Old l2tp:
  F8144374: old-l2tp.png 
  
  Old l2tp IPsec Settings :
  F8144378: old-l2tp-ipsec.png 
  
  New l2tp (username/password) :
  F8144380: new-l2tp-1.png 
  
  New l2tp (certificate) :
  F8144382: new-l2tp-2.png 
  
  New l2tp IPsec Settings (PSK) :
  F8144385: new-l2tp-ipsec-1.png 
  
  New l2tp IPsec Settings (certificate) :
  F8144388: new-l2tp-ipsec-2.png 

REPOSITORY
  R116 Plasma Network Management Applet

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

To: dkosovic, jgrulich
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27764: Update L2TP to NetworkManager-l2tp 1.8.0 features

[Douglas Kosovic]

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

REVISION SUMMARY
  - Update to NetworkManager-l2tp 1.8.0 features which include:
- NetworkManager-l2tp 1.8.0 user certificate support.
- Machine certificate support.
- Rename Gateway ID to Remote ID.
- Phase 1 & Phase 2 Lifetime TimeEdits.
- Use IP Compression CheckBox.
- Disable PFS CheckBox.
  - Synchronize nm-l2tp-service.h with NetworkManager-l2tp 1.8.0's 
nm-service-defines.h.
  - Base new user certificate UI on openvpn UI.
  - Delete l2tpauth.ui and replace with programmatically generated  
l2tpauthwidget.
  - Add user and machine private key / certificate password support to 
l2tpauthwidget.
  - Detect if libreswan or strongswan is being used and change UI based on 
which one is detected, e.g. disable "Disable PFS" check box with strongswan, 
Phase 1 & Phase 2 Lifetime show default strongswan and libreswan values when 
unchecked.
  - Disable IPSec settings button if libreswan or strongswan not detected.
  - Remove visibility of PPP Authentication group box if user certificate 
authentication is selected.
  - Support loading *SWAN Base64 encoded PSK, but for backwards compatibility 
use unencoded for saving settings.
  - For backwards compatibility with older NetworkManager-l2tp use 
`NM_L2TP_KEY_IPSEC_GATEWAY_ID` instead of `NM_L2TP_KEY_IPSEC_REMOTE_ID` for 
saving connection settings.

REPOSITORY
  R116 Plasma Network Management Applet

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

AFFECTED FILES
  vpn/l2tp/CMakeLists.txt
  vpn/l2tp/l2tp.cpp
  vpn/l2tp/l2tp.ui
  vpn/l2tp/l2tpauth.cpp
  vpn/l2tp/l2tpauth.h
  vpn/l2tp/l2tpauth.ui
  vpn/l2tp/l2tpipsec.ui
  vpn/l2tp/l2tpipsecwidget.cpp
  vpn/l2tp/l2tpipsecwidget.h
  vpn/l2tp/l2twidget.cpp
  vpn/l2tp/l2twidget.h
  vpn/l2tp/l2tpwidget.cpp
  vpn/l2tp/l2tpwidget.h
  vpn/l2tp/nm-l2tp-service.h

To: dkosovic, jgrulich
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart