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
> +        <property name="text">
> +         <string>Prevent the user from manually accepting
> +invalid certificates</string>
> +        </property>

this seems a bit too long as label for a checkbox

> openconnectprop.ui:208
> +        <property name="text">
> +         <string>Tokens</string>
> +        </property>

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

> openconnecttoken.ui:13-15
> +  <property name="windowTitle">
> +   <string>Form</string>
> +  </property>

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

> openconnecttoken.ui:85
> +         <property name="text">
> +          <string>Yubikey OATH</string>
> +         </property>

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

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

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 translatable, so these checks break when using a translation;
a better way is to use the itemData mechanism of a combobox to associate arbitrary stuff (better some values of an enum) to easily get back the information on the selected item;
also, considering that the items are added statically in the ui file and here removed at runtime depending on the available modes, then IMHO it would be better to directly only add the supported modes at runtime here

> openconnectwidget.cpp:150
>      const NMStringMap dataMap = d->setting->data();
> +    
>  

extra empty line

> openconnectwidget.cpp:163
> +    
> +    int index = d->tokenModeList.indexOf(dataMap[NM_OPENCONNECT_KEY_TOKEN_MODE]);
> +    if (index > 0) {

const

> openconnectwidget.cpp:241
> +    d->tokenDlg->show();
> +}
>  bool OpenconnectSettingWidget::isValid() const

missing empty line after this

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