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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20190406/cbbcc53e/attachment.html>