D18394: Add OTP support for openconnect VPN

[Jan Grulich]

This revision was automatically updated to reflect the committed changes.
Closed by commit R116:8fb5c6192c15: Add OTP support for openconnect VPN 
(authored by jgrulich).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D18394?vs=57394&id=57634#toc

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18394?vs=57394&id=57634

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

AFFECTED FILES
  vpn/openconnect/CMakeLists.txt
  vpn/openconnect/nm-openconnect-service.h
  vpn/openconnect/openconnectauth.cpp
  vpn/openconnect/openconnectauth.h
  vpn/openconnect/openconnectauthworkerthread.cpp
  vpn/openconnect/openconnectauthworkerthread.h
  vpn/openconnect/openconnectprop.ui
  vpn/openconnect/openconnecttoken.ui
  vpn/openconnect/openconnectwidget.cpp
  vpn/openconnect/openconnectwidget.h

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

D18394: Add OTP support for openconnect VPN

[Enrique Melendez]

enriquem updated this revision to Diff 57394.
enriquem added a comment.


  I addressed your comments. Sorry for the careless coding in what spaces are 
concerned. BTW, I added a missing QStringLiteral.
  
  I believe the combobox size constraints are fixed now (although I liked it 
better before).

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18394?vs=57294&id=57394

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

AFFECTED FILES
  vpn/openconnect/CMakeLists.txt
  vpn/openconnect/nm-openconnect-service.h
  vpn/openconnect/openconnectauth.cpp
  vpn/openconnect/openconnectauth.h
  vpn/openconnect/openconnectauthworkerthread.cpp
  vpn/openconnect/openconnectauthworkerthread.h
  vpn/openconnect/openconnectprop.ui
  vpn/openconnect/openconnectwidget.cpp
  vpn/openconnect/openconnectwidget.h

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

D18394: Add OTP support for openconnect VPN

[Jan Grulich]

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


  Just fix those minor issues, otherwise it looks good to me, I'll verify on 
Monday, but I believe it's safe to push this.

INLINE COMMENTS

> openconnectauth.cpp:287
> +ret = __openconnect_set_token_mode(d->vpninfo, 
> OC_TOKEN_MODE_STOKEN, tokenSecret);
> +} else if (d->tokenMode ==QStringLiteral("stokenrc")) {
> +ret = __openconnect_set_token_mode(d->vpninfo, 
> OC_TOKEN_MODE_STOKEN, NULL);

Missing space after "==".

> openconnectauth.cpp:293
> +#if OPENCONNECT_CHECK_VER(3,4)
> +else if (d->tokenMode ==  QStringLiteral("hotp") && 
> !tokenSecret.isEmpty()) {
> +ret = __openconnect_set_token_mode(d->vpninfo, 
> OC_TOKEN_MODE_HOTP, tokenSecret);

And here is one additional space after "=="

> openconnecttoken.ui:36
> +
> + QComboBox::AdjustToContents
> +

I think this combobox should be expanded across whole width.

> openconnectwidget.cpp:132
> +d->tokenUi.leTokenSecret->setToolTip("Insert the secret here, with a 
> sha specification and a leading '0x' or 'base32:'. See the openconnect 
> documentation for syntax.");
> +} else if (mode ==QStringLiteral("hotp")) {
> +d->tokenUi.leTokenSecret->setEnabled(true);

Missing space after "=="

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

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

D18394: Add OTP support for openconnect VPN

[Enrique Melendez]

enriquem updated this revision to Diff 57294.
enriquem added a comment.


  I believe I addressed all of your comments. Please check your email for 
testing options.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18394?vs=55714&id=57294

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

AFFECTED FILES
  vpn/openconnect/CMakeLists.txt
  vpn/openconnect/nm-openconnect-service.h
  vpn/openconnect/openconnectauth.cpp
  vpn/openconnect/openconnectauth.h
  vpn/openconnect/openconnectauthworkerthread.cpp
  vpn/openconnect/openconnectauthworkerthread.h
  vpn/openconnect/openconnectprop.ui
  vpn/openconnect/openconnecttoken.ui
  vpn/openconnect/openconnectwidget.cpp
  vpn/openconnect/openconnectwidget.h

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

D18394: Add OTP support for openconnect VPN

[Jan Grulich]

jgrulich added a comment.


  Sorry for the delay, I promise that I will start reviewing this more 
frequently so it gets merged just in time for Plasma  5.16 (we have 2 weeks).
  
  One more thing: When I open the "Token authentication" dialog, the "Token 
secret" label is not aligned with the text input
  
  Otherwise I think it looks good, but I didn't test it as I don't have time to 
configure an openconnect server.

INLINE COMMENTS

> openconnectauth.cpp:285
> +
> +if (d->tokenMode == "manual" && !tokenSecret.isEmpty()) {
> +ret = __openconnect_set_token_mode(d->vpninfo, 
> OC_TOKEN_MODE_STOKEN, tokenSecret);

QStringLiteral("manual")

Same below.

> openconnectauth.cpp:288
> +}
> +else if (d->tokenMode =="stokenrc") {
> +ret = __openconnect_set_token_mode(d->vpninfo, 
> OC_TOKEN_MODE_STOKEN, NULL);

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

> openconnectprop.ui:181
> +
> + Invalid certificates won't be acepted
> +

Use same description NM uses in nm-connection-editor:
"Prevent user from manually accepting invalid certificates"

> openconnectwidget.cpp:120
> +QVariant mode = d->tokenUi.cmbTokenMode->itemData(index);
> +if (mode == "disabled") {
> +d->tokenUi.leTokenSecret->setEnabled(false);

QStringLiteral("disabled")

Same below.

> openconnectwidget.cpp:124
> +}
> +else if (mode == "stokenrc") {
> +d->tokenUi.leTokenSecret->setEnabled(false);

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

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

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

D18394: Add OTP support for openconnect VPN

[Enrique Melendez]

enriquem updated this revision to Diff 55714.
enriquem added a comment.


  Update per comments above

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18394?vs=55653&id=55714

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

AFFECTED FILES
  vpn/openconnect/CMakeLists.txt
  vpn/openconnect/nm-openconnect-service.h
  vpn/openconnect/openconnectauth.cpp
  vpn/openconnect/openconnectauth.h
  vpn/openconnect/openconnectauthworkerthread.cpp
  vpn/openconnect/openconnectauthworkerthread.h
  vpn/openconnect/openconnectprop.ui
  vpn/openconnect/openconnecttoken.ui
  vpn/openconnect/openconnectwidget.cpp
  vpn/openconnect/openconnectwidget.h

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

D18394: Add OTP support for openconnect VPN

[Enrique Melendez]

enriquem added a comment.


  > ! In D18394#445411 , @jgrulich 
wrote:
  
  
  
  > All forms in the kcm use "Label: input" so I would like all the options to 
follow same pattern.
  
  OK.
  
  >> b) I agree on the PasswordField, although this being an OTP it really does 
not matter if anyone sees it.
  > 
  > Ok, then I guess leave it as it is now.
  > 
  >> c) No need to save it. It is used and discarded
  > 
  > Doesn't seem to be true, when I save token secret, I see it is saved in 
NetworkManager so in this case PasswordField should be used and there should be 
an option to store it either to NM or secret agent.
  
  I was wrong in the two points above. The token secret is needed for automatic 
generation of the OTP. The OTP will not be shown in the connection dialog.
  I changed the behavior, to enable password options, but that conflicts with 
NetworkManager (I guess this is like any other vpn setting).
  
  >> d) I tried putting all options in the main UI. This made the window too 
tall for the allocated space, so that resizing was necessary or the main window 
initial size ought be changed. It looked ugly to me. That's why I opted for a 
separate dialog. I can change it if you think it is important, but, again, it 
looks ugly to me.
  > 
  > In that case use a different button label, something like "Token 
Authentication".
  
  OK.

REPOSITORY
  R116 Plasma Network Management Applet

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

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

D18394: Add OTP support for openconnect VPN

[Jan Grulich]

jgrulich added a comment.


  In D18394#445224 , @enriquem wrote:
  
  > In D18394#444795 , @jgrulich 
wrote:
  >
  > > 1. I'm not sure if the UI for openconnect tokens is correct, I think the 
QLineEdit for token secret should be on the same line, and you should probably 
use PasswordField instead? It can be our PasswordField widget from 
libs/editor/widgets/. Or it's not secret in the same sense as other secrets and 
it will not need to be saved by secret agent, like rest of passwords? I would 
also follow nm-connection-editor and make tokens options visible in the main 
UI, not under specific button.
  >
  >
  > a) I don't see any need for the QComboBox and theQLineEdit to be in the 
same line, but that's a matter of taste, not functionality.  Both fields are 
sort of independent: same key works with different OTP options.
  
  
  All forms in the kcm use "Label: input" so I would like all the options to 
follow same pattern.
  
  > b) I agree on the PasswordField, although this being an OTP it really does 
not matter if anyone sees it.
  
  Ok, then I guess leave it as it is now.
  
  > c) No need to save it. It is used and discarded
  
  Doesn't seem to be true, when I save token secret, I see it is saved in 
NetworkManager so in this case PasswordField should be used and there should be 
an option to store it either to NM or secret agent.
  
  > d) I tried putting all options in the main UI. This made the window too 
tall for the allocated space, so that resizing was necessary or the main window 
initial size ought be changed. It looked ugly to me. That's why I opted for a 
separate dialog. I can change it if you think it is important, but, again, it 
looks ugly to me.
  
  In that case use a different button label, something like "Token 
Authentication".

REPOSITORY
  R116 Plasma Network Management Applet

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

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

D18394: Add OTP support for openconnect VPN

[Enrique Melendez]

enriquem added inline comments.

INLINE COMMENTS

> jgrulich wrote in nm-openconnect-service.h:41
> Leftover from the other review?

Not really. It's a feature in NetworkManager. If the functionality is not 
needed, it can be dropped.

REPOSITORY
  R116 Plasma Network Management Applet

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

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

D18394: Add OTP support for openconnect VPN

[Enrique Melendez]

enriquem updated this revision to Diff 55653.
enriquem added a comment.


  Hopefully there are no more trailing spaces.

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18394?vs=50749&id=55653

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

AFFECTED FILES
  vpn/openconnect/CMakeLists.txt
  vpn/openconnect/nm-openconnect-service.h
  vpn/openconnect/openconnectauth.cpp
  vpn/openconnect/openconnectauth.h
  vpn/openconnect/openconnectauthworkerthread.cpp
  vpn/openconnect/openconnectauthworkerthread.h
  vpn/openconnect/openconnectprop.ui
  vpn/openconnect/openconnecttoken.ui
  vpn/openconnect/openconnectwidget.cpp
  vpn/openconnect/openconnectwidget.h

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

D18394: Add OTP support for openconnect VPN

[Enrique Melendez]

enriquem added a comment.


  In D18394#444795 , @jgrulich wrote:
  
  > 1. I'm not sure if the UI for openconnect tokens is correct, I think the 
QLineEdit for token secret should be on the same line, and you should probably 
use PasswordField instead? It can be our PasswordField widget from 
libs/editor/widgets/. Or it's not secret in the same sense as other secrets and 
it will not need to be saved by secret agent, like rest of passwords? I would 
also follow nm-connection-editor and make tokens options visible in the main 
UI, not under specific button.
  
  
  a) I don't see any need for the QComboBox and theQLineEdit to be in the same 
line, but that's a matter of taste, not functionality.  Both fields are sort of 
independent: same key works with different OTP options. 
  b) I agree on the PasswordField, although this being an OTP it really does 
not matter if anyone sees it.
  c) No need to save it. It is used and discarded
  d) I tried putting all optins in the main UI. This made the window too tall 
for the allocated space, so that resizing was necessary or the main window 
initial size ought be changed. It looked ugly to me. That's why I opted for a 
separate dialog. I can change it if you think it is important, but, again, it 
looks ugly to me.
  
  > 2. Your code is full of trailing spaces
  
  Ah, well, what a curse! I'll get rid of them
  
  > 3. How can I try this? Is there any public Openconnect server which I can 
use to test this?
  
  I set up a server in my own Fedora box with ocserv. With some tweaking of the 
pam modules along the lines of 
http://ocserv.gitlab.io/www/recipes-ocserv-2fa.html, 
https://www.nongnu.org/oath-toolkit/pam_oath.html and  
http://www.infradead.org/openconnect/token.html I was able to test HOTP and 
TOTP (that is, I pick a random key and use oathtool or FreeOTP). Yubikeys were 
triky, since I couldn't validate the OTP. But I modified ocserv to show that 
the connection scripts were actually providing the correct OTP key. As for RSA, 
I have no clue as to how to test them, and keys are too expensive for me.

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

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

D18394: Add OTP support for openconnect VPN

[Jan Grulich]

jgrulich added a comment.


  1. I'm not sure if the UI for openconnect tokens is correct, I think the 
QLineEdit for token secret should be on the same line, and you should probably 
use PasswordField instead? It can be our PasswordField widget from 
libs/editor/widgets/. Or it's not secret in the same sense as other secrets and 
it will not need to be saved by secret agent, like rest of passwords? I would 
also follow nm-connection-editor and make tokens options visible in the main 
UI, not under specific button.
  2. Your code is full of trailing spaces
  3. How can I try this? Is there any public Openconnect server which I can use 
to test this?

INLINE COMMENTS

> nm-openconnect-service.h:41
>  #define NM_OPENCONNECT_KEY_PEM_PASSPHRASE_FSID "pem_passphrase_fsid"
> +#define NM_OPENCONNECT_KEY_PREVENT_INVALID_CERT "prevent_invalid_cert"
>  #define NM_OPENCONNECT_KEY_PROTOCOL "protocol"

Leftover from the other review?

> openconnectauth.cpp:659
> +
> +const NMStringMap dataMap = d->setting->data();
> +
> buttons->button(QDialogButtonBox::Ok)->setEnabled(dataMap[NM_OPENCONNECT_KEY_PREVENT_INVALID_CERT]
>  != "yes");

Also leftover from the other review.

> openconnectwidget.cpp:95
> +// Just popping up the tokenDlg changes nothing
> +disconnect(d->ui.buTokens, &QPushButton::clicked, this, 
> &SettingWidget::settingChanged);
> +// User cancels means nothing should change here

Both disconnects can be removed if you move this all to the main widget.

> openconnectwidget.cpp:199
>  
> d->ui.chkUseFsid->setChecked(dataMap[NM_OPENCONNECT_KEY_PEM_PASSPHRASE_FSID] 
> == "yes");
> +
> d->ui.preventInvalidCert->setChecked(dataMap[NM_OPENCONNECT_KEY_PREVENT_INVALID_CERT]
>  == "yes");
> +

Leftover from the other review.

> openconnectwidget.cpp:256
>  data.insert(QLatin1String(NM_OPENCONNECT_KEY_PEM_PASSPHRASE_FSID), 
> d->ui.chkUseFsid->isChecked() ? "yes" : "no");
> +data.insert(QLatin1String(NM_OPENCONNECT_KEY_PREVENT_INVALID_CERT), 
> d->ui.preventInvalidCert->isChecked() ? "yes" : "no");
>  

Also leftover from the other review.

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

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

D18394: Add OTP support for openconnect VPN

[Enrique Melendez]

enriquem updated this revision to Diff 50749.
enriquem added a comment.


  Changed tooltips

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18394?vs=50698&id=50749

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

AFFECTED FILES
  vpn/openconnect/CMakeLists.txt
  vpn/openconnect/nm-openconnect-service.h
  vpn/openconnect/openconnectauth.cpp
  vpn/openconnect/openconnectauth.h
  vpn/openconnect/openconnectauthworkerthread.cpp
  vpn/openconnect/openconnectauthworkerthread.h
  vpn/openconnect/openconnectprop.ui
  vpn/openconnect/openconnecttoken.ui
  vpn/openconnect/openconnectwidget.cpp
  vpn/openconnect/openconnectwidget.h

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

D18394: Add OTP support for openconnect VPN

[Enrique Melendez]

enriquem updated this revision to Diff 50698.
enriquem added a comment.


  Changed tooltips
  No need for a new enum

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18394?vs=50665&id=50698

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

AFFECTED FILES
  vpn/openconnect/CMakeLists.txt
  vpn/openconnect/nm-openconnect-service.h
  vpn/openconnect/openconnectauth.cpp
  vpn/openconnect/openconnectauth.h
  vpn/openconnect/openconnectauthworkerthread.cpp
  vpn/openconnect/openconnectauthworkerthread.h
  vpn/openconnect/openconnectprop.ui
  vpn/openconnect/openconnecttoken.ui
  vpn/openconnect/openconnectwidget.cpp
  vpn/openconnect/openconnectwidget.h

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

D18394: Add OTP support for openconnect VPN

[Enrique Melendez]

enriquem updated this revision to Diff 50665.
enriquem added a comment.
Herald added 1 blocking reviewer(s): jgrulich.


  Removed updateLog calls
  Properly initialize d->tokens
  Change the config dialog so that (a) 'Apply' does not become active when the 
user clicks on the 'Show Tokens' button, makes no changes, and closes the 
dialog or clicks Cancel, and (b) when the user clicks 'Cancel', inputs read 
from the config file are restored

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18394?vs=50279&id=50665

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

AFFECTED FILES
  vpn/openconnect/CMakeLists.txt
  vpn/openconnect/nm-openconnect-service.h
  vpn/openconnect/openconnectauth.cpp
  vpn/openconnect/openconnectauth.h
  vpn/openconnect/openconnectauthworkerthread.cpp
  vpn/openconnect/openconnectauthworkerthread.h
  vpn/openconnect/openconnectprop.ui
  vpn/openconnect/openconnecttoken.ui
  vpn/openconnect/openconnectwidget.cpp
  vpn/openconnect/openconnectwidget.h

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

D18394: Add OTP support for openconnect VPN

[Enrique Melendez]

enriquem updated this revision to Diff 50279.
enriquem added a comment.


  Corrects minor bug in one call to updateLog. However, all calls to updateLog 
have to be reviewed if this goes to production

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18394?vs=49949&id=50279

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

AFFECTED FILES
  vpn/openconnect/CMakeLists.txt
  vpn/openconnect/nm-openconnect-service.h
  vpn/openconnect/openconnectauth.cpp
  vpn/openconnect/openconnectauth.h
  vpn/openconnect/openconnectauthworkerthread.cpp
  vpn/openconnect/openconnectauthworkerthread.h
  vpn/openconnect/openconnectprop.ui
  vpn/openconnect/openconnecttoken.ui
  vpn/openconnect/openconnectwidget.cpp
  vpn/openconnect/openconnectwidget.h

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

D18394: Add OTP support for openconnect VPN

[Enrique Melendez]

enriquem updated this revision to Diff 49949.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18394?vs=49946&id=49949

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

AFFECTED FILES
  vpn/openconnect/CMakeLists.txt
  vpn/openconnect/nm-openconnect-service.h
  vpn/openconnect/openconnectauth.cpp
  vpn/openconnect/openconnectauth.h
  vpn/openconnect/openconnectauthworkerthread.cpp
  vpn/openconnect/openconnectauthworkerthread.h
  vpn/openconnect/openconnectprop.ui
  vpn/openconnect/openconnecttoken.ui
  vpn/openconnect/openconnectwidget.cpp
  vpn/openconnect/openconnectwidget.h

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

D18394: Add OTP support for openconnect VPN

[Enrique Melendez]

enriquem updated this revision to Diff 49946.
enriquem added a comment.


  Changed the buildup of the "tokens" ComoBox

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18394?vs=49921&id=49946

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

AFFECTED FILES
  vpn/openconnect/CMakeLists.txt
  vpn/openconnect/nm-openconnect-service.h
  vpn/openconnect/openconnectauth.cpp
  vpn/openconnect/openconnectauth.h
  vpn/openconnect/openconnectauthworkerthread.cpp
  vpn/openconnect/openconnectauthworkerthread.h
  vpn/openconnect/openconnectprop.ui
  vpn/openconnect/openconnecttoken.ui
  vpn/openconnect/openconnectwidget.cpp
  vpn/openconnect/openconnectwidget.h

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

D18394: Add OTP support for openconnect VPN

[Enrique Melendez]

enriquem updated this revision to Diff 49921.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18394?vs=49920&id=49921

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

AFFECTED FILES
  vpn/openconnect/CMakeLists.txt
  vpn/openconnect/nm-openconnect-service.h
  vpn/openconnect/openconnectauth.cpp
  vpn/openconnect/openconnectauth.h
  vpn/openconnect/openconnectauthworkerthread.cpp
  vpn/openconnect/openconnectauthworkerthread.h
  vpn/openconnect/openconnectprop.ui
  vpn/openconnect/openconnecttoken.ui
  vpn/openconnect/openconnectwidget.cpp
  vpn/openconnect/openconnectwidget.h

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

D18394: Add OTP support for openconnect VPN

[Enrique Melendez]

enriquem updated this revision to Diff 49920.
enriquem added a comment.


  Amended to remove global tokenModeList

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18394?vs=49911&id=49920

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

AFFECTED FILES
  vpn/openconnect/CMakeLists.txt
  vpn/openconnect/nm-openconnect-service.h
  vpn/openconnect/openconnectauth.cpp
  vpn/openconnect/openconnectauth.h
  vpn/openconnect/openconnectauthworkerthread.cpp
  vpn/openconnect/openconnectauthworkerthread.h
  vpn/openconnect/openconnectprop.ui
  vpn/openconnect/openconnecttoken.ui
  vpn/openconnect/openconnectwidget.cpp
  vpn/openconnect/openconnectwidget.h

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

D18394: Add OTP support for openconnect VPN

[Enrique Melendez]

enriquem updated this revision to Diff 49911.
enriquem added a comment.


  I addressed your comments, except the ones regarding macro definitions, since 
I am unsure thar applicable versions of openconnect have compatible parameters. 
I will find out and revisit the comment later

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18394?vs=49906&id=49911

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

AFFECTED FILES
  vpn/openconnect/CMakeLists.txt
  vpn/openconnect/nm-openconnect-service.h
  vpn/openconnect/openconnectauth.cpp
  vpn/openconnect/openconnectauth.h
  vpn/openconnect/openconnectauthworkerthread.cpp
  vpn/openconnect/openconnectauthworkerthread.h
  vpn/openconnect/openconnectprop.ui
  vpn/openconnect/openconnecttoken.ui
  vpn/openconnect/openconnectwidget.cpp
  vpn/openconnect/openconnectwidget.h

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

D18394: Add OTP support for openconnect VPN

[Pino Toscano]

pino added a comment.


  Hi Enrique,
  
  I'm not a plasma-nm developer, however I provide some tips & hints regarding 
your patch.
  
  Other than what I noted already, there are few more things that apply in 
general:
  
  - make sure to respect the indentation: each level by 4 spaces with no tabs, 
space after a comma for function arguments, etc
  - make sure to use curly brackets {...} also for blocks with only 1 line
  - nullptr instead of NULL

INLINE COMMENTS

> openconnectauth.cpp:60-66
> +#if !OPENCONNECT_CHECK_VER(2,1)
> +#define __openconnect_set_token_mode(...) -EOPNOTSUPP
> +#elif !OPENCONNECT_CHECK_VER(2,2)
> +#define __openconnect_set_token_mode(vpninfo, mode, secret) 
> openconnect_set_stoken_mode(vpninfo, 1, secret)
> +#else
> +#define __openconnect_set_token_mode openconnect_set_token_mode
> +#endif

IMHO it is better to define the macros in the same way for all the cases, 
specifying their parameters

> openconnectauth.cpp:225
> +d->tokenMode = dataMap[NM_OPENCONNECT_KEY_TOKEN_MODE].toUtf8();
> +
>  }

extra empty line

> openconnectauth.cpp:279
> +
> +QByteArray tokenSecret = 
> d->secrets[NM_OPENCONNECT_KEY_TOKEN_SECRET].toUtf8();
> +if (!d->tokenMode.isEmpty()) {

this is needed only in the block within the if in the line below, so move it 
inside that block; also, make tokenSecret const

> openconnectauth.cpp:298
> +d->token.tokenMode = OC_TOKEN_MODE_YUBIOATH;
> +if (!tokenSecret.isEmpty() && tokenSecret.length())
> +d->token.tokenSecret = tokenSecret;

if tokenSecret is not empty, then its length will always be greater than 0, so 
the second condition is redundant

> openconnectauth.cpp:301
> +else
> +d->token.tokenSecret = NULL;
> +}

d->token.tokenSecret is a QByteArray, so if you want to clear it just call 
clear()

> openconnectauth.cpp:305
> +if (ret) {
> +addFormInfo(QLatin1String("dialog-error"), i18n("Failed to 
> initialize software token: %1\n", ret));
> +}

it looks like all the other messages for addFormInfo() do not have a trailing 
newline, so please remove it

> openconnectauth.cpp:360
>  addFormInfo(QLatin1String("dialog-information"), i18n("Contacting host, 
> please wait..."));
> +
>  d->worker->start();

extra empty line

> openconnectauth.cpp:374
>  {
> +
>  Q_D(const OpenconnectAuthWidget);

extra empty line

> openconnectauth.cpp:380
>  
> +
>  secrets.unite(d->secrets);

extra empty line

> openconnectauth.h:58
>  void processAuthForm(struct oc_auth_form *);
> -void updateLog(const QString &, const int &);
> +void updateLog(const QString&, const int&);
>  void logLevelChanged(int);

unrelated change

> openconnectauth.h:66
> +void initTokens();
> +
>  };

extra empty line

> openconnectauthworkerthread.h:95
> +void initTokens(void);
> +
>  protected:

extra empty line

> openconnectprop.ui:181-182
> +
> + Prevent the user from manually accepting
> +invalid certificates
> +

this seems a bit too long as label for a checkbox

> openconnectprop.ui:208
> +
> + Tokens
> +

usually buttons are actions, so IMHO "Show Tokens" is more appropriate

> openconnecttoken.ui:13-15
> +  
> +   Form
> +  

remove these lines (or invoke on this .ui file the fixuifiles script available 
in kde-dev-scripts)

> openconnecttoken.ui:85
> + 
> +  Yubikey OATH
> + 

"YubiKey"; also, is "OATH" correct?

> openconnectwidget.cpp:48
> +
> +#include 
> +

this include should be placed together with the other Qt includes some lines 
above

> openconnectwidget.cpp:56
>  NetworkManager::VpnSetting::Ptr setting;
> +mutable QStringList tokenModeList;
> +QDialog *tokenDlg;

are you sure it needs to be mutable?

> openconnectwidget.cpp:78
> +  
> +
> +d->tokenDlg = new QDialog(this);

extra empty line

> openconnectwidget.cpp:80
> +d->tokenDlg = new QDialog(this);
> +d->tokenWid = new QWidget(this);
> +d->tokenUi.setupUi(d->tokenWid);

tokenWid is not needed, you can just setupUi on the dialog

> openconnectwidget.cpp:85
> +d->tokenDlg->setLayout(layout);
> +QDialogButtonBox* buttons = new 
> QDialogButtonBox(QDialogButtonBox::Ok|QDialogButtonBox::Cancel, d->tokenDlg);
> +connect(buttons, &QDialogButtonBox::accepted, d->tokenDlg, 
> &QDialog::accept);

the button box can be added directly to the ui file, and avoid the need to add 
it manually

> openconnectwidget.cpp:122-136
> +if (combo->itemText(i).startsWith("RSA") && 
> !openconnect_has_stoken_support ()) {
> +combo->removeItem(i);
> +d->tokenModeList.removeAt(i);
> +}
> +else if (combo->itemText(i).startsWith("TOTP") && 
> !openconnect_has_oath_support ()) {
> +combo->removeItem(i);
> + d->tokenModeList.removeAt(i);

never ever compare to ui strings! they are tran

D18394: Add OTP support for openconnect VPN

[Enrique Melendez]

enriquem updated this revision to Diff 49906.

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18394?vs=49899&id=49906

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

AFFECTED FILES
  vpn/openconnect/CMakeLists.txt
  vpn/openconnect/nm-openconnect-service.h
  vpn/openconnect/openconnectauth.cpp
  vpn/openconnect/openconnectauth.h
  vpn/openconnect/openconnectauthworkerthread.cpp
  vpn/openconnect/openconnectauthworkerthread.h
  vpn/openconnect/openconnectprop.ui
  vpn/openconnect/openconnecttoken.ui
  vpn/openconnect/openconnectwidget.cpp
  vpn/openconnect/openconnectwidget.h

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

D18394: Add OTP support for openconnect VPN

[Enrique Melendez]

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

REVISION SUMMARY
  With this patch, One Time Password (OTP) support is added to the openconnect 
VPN settings and code. It draws the specifications from 
NetworkManager-openconnect, and presents the same functionality. 
  The config dialog is modified to include settings for the OTP options
  The auth dialog reads and sets the OTP option, updating tokens as needed

REPOSITORY
  R116 Plasma Network Management Applet

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

AFFECTED FILES
  vpn/openconnect/CMakeLists.txt
  vpn/openconnect/nm-openconnect-service.h
  vpn/openconnect/openconnectauth.cpp
  vpn/openconnect/openconnectauth.h
  vpn/openconnect/openconnectauthworkerthread.cpp
  vpn/openconnect/openconnectauthworkerthread.h
  vpn/openconnect/openconnectprop.ui
  vpn/openconnect/openconnecttoken.ui
  vpn/openconnect/openconnectwidget.cpp
  vpn/openconnect/openconnectwidget.h

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