Differential D18394 Diff 50279 vpn/openconnect/openconnectwidget.cpp

Changeset View

Standalone View

View Options

vpn/openconnect/openconnectwidget.cpp

Context not available.
22#include "openconnectwidget.h" 22​#include "openconnectwidget.h"
23#include <QDialog> 23​#include <QDialog>
24#include <QUrl> 24​#include <QUrl>
25​#include <QStringList>
25 26​
26#include "ui_openconnectprop.h" 27​#include "ui_openconnectprop.h"
28​#include "ui_openconnecttoken.h"
27 29​
28#include <QString> 30​#include <QString>
31​#include <QDialogButtonBox>
29#include "nm-openconnect-service.h" 32​#include "nm-openconnect-service.h"
30 33​
34​#include <openconnect.h>
35​#ifndef OPENCONNECT_CHECK_VER
36​#define OPENCONNECT_CHECK_VER(x,y) 0
37​#endif
38​
39​#if !OPENCONNECT_CHECK_VER(2,1)
40​#define openconnect_has_stoken_support() 0
41​#endif
42​#if !OPENCONNECT_CHECK_VER(2,2)
43​#define openconnect_has_oath_support() 0
44​#endif
45​#if !OPENCONNECT_CHECK_VER(5,0)
46​#define openconnect_has_yubioath_support() 0
47​#endif
48​
pinoUnsubmitted
Not Done

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

pino: this include should be placed together with the other Qt includes some lines above
31class OpenconnectSettingWidgetPrivate 49​class OpenconnectSettingWidgetPrivate
32{ 50​{
33public: 51​public:
34 Ui_OpenconnectProp ui; 52​ Ui_OpenconnectProp ui;
53​ Ui::OpenConnectToken tokenUi;
35 NetworkManager::VpnSetting::Ptr setting; 54​ NetworkManager::VpnSetting::Ptr setting;
55​ QDialog *tokenDlg;
36}; 56​};
pinoUnsubmitted
Not Done

are you sure it needs to be mutable?

pino: are you sure it needs to be mutable?
37 57​
38OpenconnectSettingWidget::OpenconnectSettingWidget(const NetworkManager::VpnSetting::Ptr &setting, QWidget * parent) 58​OpenconnectSettingWidget::OpenconnectSettingWidget(const NetworkManager::VpnSetting::Ptr &setting, QWidget * parent)
Context not available.
48 68​
49 // Connect for validity check 69​ // Connect for validity check
50 connect(d->ui.leGateway, &QLineEdit::textChanged, this, &OpenconnectSettingWidget::slotWidgetChanged); 70​ connect(d->ui.leGateway, &QLineEdit::textChanged, this, &OpenconnectSettingWidget::slotWidgetChanged);
71​ connect(d->ui.buTokens, &QPushButton::clicked, this, &OpenconnectSettingWidget::showTokens);
72​
73​ d->tokenDlg = new QDialog(this);
74​ d->tokenUi.setupUi(d->tokenDlg);
75​ QVBoxLayout * layout = new QVBoxLayout(d->tokenDlg);
76​ layout->addWidget(d->tokenDlg);
77​ d->tokenDlg->setLayout(layout);
78​ connect(d->tokenUi.buttonBox, &QDialogButtonBox::accepted, d->tokenDlg, &QDialog::accept);
pinoUnsubmitted
Not Done

extra empty line

pino: extra empty line
79​ connect(d->tokenUi.buttonBox, &QDialogButtonBox::rejected, d->tokenDlg, &QDialog::reject);
80​
pinoUnsubmitted
Not Done

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

pino: tokenWid is not needed, you can just setupUi on the dialog
81​ connect(d->tokenUi.cmbTokenMode, QOverload<int>::of(&QComboBox::currentIndexChanged), this, QOverload<int>::of((&OpenconnectSettingWidget::enableTokenSecret)));
82​ connect(d->tokenUi.leTokenSecret, &QPlainTextEdit::textChanged, this, &OpenconnectSettingWidget::slotWidgetChanged);
83​ d->tokenUi.gbToken->setVisible(initTokenGroup());
51 84​
52 KAcceleratorManager::manage(this); 85​ KAcceleratorManager::manage(this);
pinoUnsubmitted
Not Done

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

pino: the button box can be added directly to the ui file, and avoid the need to add it manually
53 86​
Context not available.
61 delete d_ptr; 94​ delete d_ptr;
62} 95​}
jgrulichUnsubmitted
Not Done

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

jgrulich: Both disconnects can be removed if you move this all to the main widget.
63 96​
97​void OpenconnectSettingWidget::enableTokenSecret(int index)
98​{
99​ Q_D(const OpenconnectSettingWidget);
100​
101​ d->tokenUi.leTokenSecret->setEnabled(index > 1);
102​}
103​
104​bool OpenconnectSettingWidget::initTokenGroup()
105​{
106​ Q_D(const OpenconnectSettingWidget);
107​
108​ int validRows = 0;
109​ QStringList tokenLabelList = QStringList() << "Disabled" << "RSA SecurID — read from ~/.stokenrc" << "RSA SecurID — manually entered" << "TOTP — manually entered" << "HOTP — manually entered" << "Yubikey";
110​ QStringList tokenModeList = QStringList() << "disabled" << "stokenrc" << "manual" << "totp" << "hotp" << "yubioath";
111​ QComboBox *combo = d->tokenUi.cmbTokenMode;
112​
113​ combo->addItem(tokenLabelList[validRows]);
114​ combo->setItemData(validRows, tokenModeList[validRows], Qt::UserRole);
115​ validRows++;
116​ if (openconnect_has_stoken_support ()) {
117​ for ( ; validRows < 3; validRows++) {
118​ combo->addItem(tokenLabelList[validRows]);
119​ combo->setItemData(validRows, tokenModeList[validRows], Qt::UserRole);
120​ }
jgrulichUnsubmitted
Not Done

QStringLiteral("disabled")

Same below.

jgrulich: QStringLiteral("disabled") Same below.
121​ }
122​ if (openconnect_has_oath_support ()) {
123​ combo->addItem(tokenLabelList[validRows]);
124​ combo->setItemData(validRows, tokenModeList[validRows], Qt::UserRole);
jgrulichUnsubmitted
Not Done

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

jgrulich: Coding style. The "else if" should be on the same line as the "}" bracket. Same below.
125​ validRows++;
126​ if (OPENCONNECT_CHECK_VER(3,4)) {
127​ combo->addItem(tokenLabelList[validRows]);
128​ combo->setItemData(validRows, tokenModeList[validRows], Qt::UserRole);
129​ validRows++;
130​ }
131​ }
132​ if (openconnect_has_yubioath_support ()) {
jgrulichUnsubmitted
Not Done

Missing space after "=="

jgrulich: Missing space after "=="
133​ combo->addItem(tokenLabelList[validRows]);
134​ combo->setItemData(validRows, tokenModeList[validRows], Qt::UserRole);
135​ }
136​ return validRows > 0;
pinoUnsubmitted
Not Done

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

pino: never ever compare to ui strings! they are translatable, so these checks break when using a…
137​}
138​
64void OpenconnectSettingWidget::loadConfig(const NetworkManager::Setting::Ptr &setting) 139​void OpenconnectSettingWidget::loadConfig(const NetworkManager::Setting::Ptr &setting)
65{ 140​{
66 Q_D(OpenconnectSettingWidget); 141​ Q_D(OpenconnectSettingWidget);
67 Q_UNUSED(setting) ​
68 142​
69 // General settings 143​ // General settings
70 const NMStringMap dataMap = d->setting->data(); 144​ const NMStringMap dataMap = d->setting->data();
pinoUnsubmitted
Not Done

extra empty line

pino: extra empty line
Context not available.
78 d->ui.leUserCert->setUrl(QUrl::fromLocalFile(dataMap[NM_OPENCONNECT_KEY_USERCERT])); 152​ d->ui.leUserCert->setUrl(QUrl::fromLocalFile(dataMap[NM_OPENCONNECT_KEY_USERCERT]));
79 d->ui.leUserPrivateKey->setUrl(QUrl::fromLocalFile(dataMap[NM_OPENCONNECT_KEY_PRIVKEY])); 153​ d->ui.leUserPrivateKey->setUrl(QUrl::fromLocalFile(dataMap[NM_OPENCONNECT_KEY_PRIVKEY]));
80 d->ui.chkUseFsid->setChecked(dataMap[NM_OPENCONNECT_KEY_PEM_PASSPHRASE_FSID] == "yes"); 154​ d->ui.chkUseFsid->setChecked(dataMap[NM_OPENCONNECT_KEY_PEM_PASSPHRASE_FSID] == "yes");
155​ d->ui.preventInvalidCert->setChecked(dataMap[NM_OPENCONNECT_KEY_PREVENT_INVALID_CERT] == "yes");
156​
157​ for (int index = 0; index < d->tokenUi.cmbTokenMode->count(); index++) {
158​ if (d->tokenUi.cmbTokenMode->itemData(index, Qt::UserRole) == dataMap[NM_OPENCONNECT_KEY_TOKEN_MODE]) {
159​ d->tokenUi.cmbTokenMode->setCurrentIndex(index);
160​ if (index > 1) {
161​ loadSecrets(setting);
162​ }
163​ break;
pinoUnsubmitted
Not Done

const

pino: const
164​ }
165​ }
166​}
167​
168​void OpenconnectSettingWidget::loadSecrets(const NetworkManager::Setting::Ptr &setting)
169​{
170​ Q_D(OpenconnectSettingWidget);
171​
172​ NetworkManager::VpnSetting::Ptr vpnSetting = setting.staticCast<NetworkManager::VpnSetting>();
173​
174​ if (vpnSetting) {
175​ const NMStringMap secrets = vpnSetting->secrets();
176​ d->tokenUi.leTokenSecret->appendPlainText(secrets.value(NM_OPENCONNECT_KEY_TOKEN_SECRET));
177​ }
81} 178​}
82 179​
83QVariantMap OpenconnectSettingWidget::setting() const 180​QVariantMap OpenconnectSettingWidget::setting() const
Context not available.
88 setting.setServiceType(QLatin1String(NM_DBUS_SERVICE_OPENCONNECT)); 185​ setting.setServiceType(QLatin1String(NM_DBUS_SERVICE_OPENCONNECT));
89 186​
90 NMStringMap data; 187​ NMStringMap data;
188​ NMStringMap secrets;
91 189​
92 data.insert(NM_OPENCONNECT_KEY_PROTOCOL, d->ui.cmbProtocol->currentIndex() ? QLatin1String("nc") : QLatin1String("anyconnect")); 190​ data.insert(NM_OPENCONNECT_KEY_PROTOCOL, d->ui.cmbProtocol->currentIndex() ? QLatin1String("nc") : QLatin1String("anyconnect"));
93 data.insert(QLatin1String(NM_OPENCONNECT_KEY_GATEWAY), d->ui.leGateway->text()); 191​ data.insert(QLatin1String(NM_OPENCONNECT_KEY_GATEWAY), d->ui.leGateway->text());
jgrulichUnsubmitted
Not Done

Leftover from the other review.

jgrulich: Leftover from the other review.
Context not available.
108 data.insert(QLatin1String(NM_OPENCONNECT_KEY_PRIVKEY), d->ui.leUserPrivateKey->url().toLocalFile()); 206​ data.insert(QLatin1String(NM_OPENCONNECT_KEY_PRIVKEY), d->ui.leUserPrivateKey->url().toLocalFile());
109 } 207​ }
110 data.insert(QLatin1String(NM_OPENCONNECT_KEY_PEM_PASSPHRASE_FSID), d->ui.chkUseFsid->isChecked() ? "yes" : "no"); 208​ data.insert(QLatin1String(NM_OPENCONNECT_KEY_PEM_PASSPHRASE_FSID), d->ui.chkUseFsid->isChecked() ? "yes" : "no");
209​ data.insert(QLatin1String(NM_OPENCONNECT_KEY_PREVENT_INVALID_CERT), d->ui.preventInvalidCert->isChecked() ? "yes" : "no");
210​
211​ int index = d->tokenUi.cmbTokenMode->currentIndex();
212​ data.insert(QLatin1String(NM_OPENCONNECT_KEY_TOKEN_MODE), d->tokenUi.cmbTokenMode->itemData(index, Qt::UserRole).toString());
213​ secrets.insert(QLatin1String(NM_OPENCONNECT_KEY_TOKEN_SECRET), d->tokenUi.leTokenSecret->toPlainText());
111 214​
112 // Restore previous flags, this is necessary for keeping secrets stored in KWallet 215​ // Restore previous flags, this is necessary for keeping secrets stored in KWallet
113 Q_FOREACH (const QString &key, d->setting->data().keys()) { 216​ Q_FOREACH (const QString &key, d->setting->data().keys()) {
Context not available.
122 data.insert(QLatin1String(NM_OPENCONNECT_KEY_GATEWAY"-flags"), QString::number(NetworkManager::Setting::NotSaved)); 225​ data.insert(QLatin1String(NM_OPENCONNECT_KEY_GATEWAY"-flags"), QString::number(NetworkManager::Setting::NotSaved));
123 226​
124 setting.setData(data); 227​ setting.setData(data);
125 setting.setSecrets(d->setting->secrets()); 228​ setting.setSecrets(secrets);
126 229​
127 return setting.toMap(); 230​ return setting.toMap();
128} 231​}
129 232​
233​void OpenconnectSettingWidget::showTokens()
234​{
235​ Q_D(OpenconnectSettingWidget);
236​
237​ d->tokenDlg->show();
238​}
130bool OpenconnectSettingWidget::isValid() const 239​bool OpenconnectSettingWidget::isValid() const
131{ 240​{
132 Q_D(const OpenconnectSettingWidget); 241​ Q_D(const OpenconnectSettingWidget);
pinoUnsubmitted
Not Done

missing empty line after this

pino: missing empty line after this
242​
133 return !d->ui.leGateway->text().isEmpty(); 243​ return !d->ui.leGateway->text().isEmpty();
134} 244​}
Context not available.
jgrulichUnsubmitted
Not Done

Also leftover from the other review.

jgrulich: Also leftover from the other review.