Differential D18394 Diff 49906 vpn/openconnect/openconnectwidget.cpp

Changeset View

Standalone View

View Options

vpn/openconnect/openconnectwidget.cpp

Context not available.
24#include <QUrl> 24​#include <QUrl>
25 25​
26#include "ui_openconnectprop.h" 26​#include "ui_openconnectprop.h"
27​#include "ui_openconnecttoken.h"
27 28​
28#include <QString> 29​#include <QString>
30​#include <QDialogButtonBox>
29#include "nm-openconnect-service.h" 31​#include "nm-openconnect-service.h"
30 32​
33​#include <openconnect.h>
34​#ifndef OPENCONNECT_CHECK_VER
35​#define OPENCONNECT_CHECK_VER(x,y) 0
36​#endif
37​
38​#if !OPENCONNECT_CHECK_VER(2,1)
39​#define openconnect_has_stoken_support() 0
40​#endif
41​#if !OPENCONNECT_CHECK_VER(2,2)
42​#define openconnect_has_oath_support() 0
43​#endif
44​#if !OPENCONNECT_CHECK_VER(5,0)
45​#define openconnect_has_yubioath_support() 0
46​#endif
47​
48​#include <QStringList>
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
49​
31class OpenconnectSettingWidgetPrivate 50​class OpenconnectSettingWidgetPrivate
32{ 51​{
33public: 52​public:
34 Ui_OpenconnectProp ui; 53​ Ui_OpenconnectProp ui;
54​ Ui::OpenConnectToken tokenUi;
35 NetworkManager::VpnSetting::Ptr setting; 55​ NetworkManager::VpnSetting::Ptr setting;
56​ mutable QStringList tokenModeList;
pinoUnsubmitted
Not Done

are you sure it needs to be mutable?

pino: are you sure it needs to be mutable?
57​ QDialog *tokenDlg;
58​ QWidget *tokenWid;
36}; 59​};
37 60​
38OpenconnectSettingWidget::OpenconnectSettingWidget(const NetworkManager::VpnSetting::Ptr &setting, QWidget * parent) 61​OpenconnectSettingWidget::OpenconnectSettingWidget(const NetworkManager::VpnSetting::Ptr &setting, QWidget * parent)
Context not available.
42 Q_D(OpenconnectSettingWidget); 65​ Q_D(OpenconnectSettingWidget);
43 d->ui.setupUi(this); 66​ d->ui.setupUi(this);
44 d->setting = setting; 67​ d->setting = setting;
68​
69​ d->tokenModeList = QStringList() << "disabled" << "stokenrc" << "manual" << "totp" << "hotp" << "yubioath";
45 70​
46 // Connect for setting check 71​ // Connect for setting check
47 watchChangedSetting(); 72​ watchChangedSetting();
48 73​
49 // Connect for validity check 74​ // Connect for validity check
50 connect(d->ui.leGateway, &QLineEdit::textChanged, this, &OpenconnectSettingWidget::slotWidgetChanged); 75​ connect(d->ui.leGateway, &QLineEdit::textChanged, this, &OpenconnectSettingWidget::slotWidgetChanged);
51 76​ connect(d->ui.buTokens, &QPushButton::clicked, this, &OpenconnectSettingWidget::showTokens);
77​
78​
pinoUnsubmitted
Not Done

extra empty line

pino: extra empty line
79​ d->tokenDlg = new QDialog(this);
80​ d->tokenWid = new QWidget(this);
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​ d->tokenUi.setupUi(d->tokenWid);
82​ QVBoxLayout * layout = new QVBoxLayout(d->tokenDlg);
83​ layout->addWidget(d->tokenWid);
84​ d->tokenDlg->setLayout(layout);
85​ QDialogButtonBox* buttons = new QDialogButtonBox(QDialogButtonBox::Ok|QDialogButtonBox::Cancel, d->tokenDlg);
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
86​ connect(buttons, &QDialogButtonBox::accepted, d->tokenDlg, &QDialog::accept);
87​ connect(buttons, &QDialogButtonBox::rejected, d->tokenDlg, &QDialog::reject);
88​ layout->addWidget(buttons);
89​
90​ connect(d->tokenUi.cmbTokenMode, QOverload<int>::of(&QComboBox::currentIndexChanged), this, QOverload<int>::of((&OpenconnectSettingWidget::enableTokenSecret)));
91​ connect(d->tokenUi.leTokenSecret, &QPlainTextEdit::textChanged, this, &OpenconnectSettingWidget::slotWidgetChanged);
92​ d->tokenUi.gbToken->setVisible(initTokenGroup());
93​
52 KAcceleratorManager::manage(this); 94​ KAcceleratorManager::manage(this);
53 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.
54 if (d->setting) { 96​ if (d->setting) {
Context not available.
61 delete d_ptr; 103​ delete d_ptr;
62} 104​}
63 105​
106​void OpenconnectSettingWidget::enableTokenSecret(int index)
107​{
108​ Q_D(const OpenconnectSettingWidget);
109​
110​ d->tokenUi.leTokenSecret->setEnabled(index > 1);
111​}
112​
113​bool OpenconnectSettingWidget::initTokenGroup()
114​{
115​ Q_D(const OpenconnectSettingWidget);
116​ int validRows = 0;
117​
118​ // iterate through the items and find the ones to be removed
119​ QComboBox *combo = d->tokenUi.cmbTokenMode;
120​
jgrulichUnsubmitted
Not Done

QStringLiteral("disabled")

Same below.

jgrulich: QStringLiteral("disabled") Same below.
121​ for (int i = combo->count()-1; i>=0 ; --i) {
122​ if (combo->itemText(i).startsWith("RSA") && !openconnect_has_stoken_support ()) {
123​ combo->removeItem(i);
124​ d->tokenModeList.removeAt(i);
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​ }
126​ else if (combo->itemText(i).startsWith("TOTP") && !openconnect_has_oath_support ()) {
127​ combo->removeItem(i);
128​ d->tokenModeList.removeAt(i);
129​ }
130​ else if (combo->itemText(i).startsWith("HOTP") && !openconnect_has_oath_support ()) {
131​ combo->removeItem(i);
132​ d->tokenModeList.removeAt(i);
jgrulichUnsubmitted
Not Done

Missing space after "=="

jgrulich: Missing space after "=="
133​ }
134​ else if (combo->itemText(i).startsWith("Yubikey") && !openconnect_has_yubioath_support ()) {
135​ combo->removeItem(i);
136​ d->tokenModeList.removeAt(i);
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​ else
139​ ++validRows;
140​ }
141​ return validRows > 1;
142​}
143​
64void OpenconnectSettingWidget::loadConfig(const NetworkManager::Setting::Ptr &setting) 144​void OpenconnectSettingWidget::loadConfig(const NetworkManager::Setting::Ptr &setting)
65{ 145​{
66 Q_D(OpenconnectSettingWidget); 146​ Q_D(OpenconnectSettingWidget);
67 Q_UNUSED(setting) ​
68 147​
69 // General settings 148​ // General settings
70 const NMStringMap dataMap = d->setting->data(); 149​ const NMStringMap dataMap = d->setting->data();
150​
pinoUnsubmitted
Not Done

extra empty line

pino: extra empty line
71 151​
72 d->ui.cmbProtocol->setCurrentIndex(dataMap[NM_OPENCONNECT_KEY_PROTOCOL] != QLatin1String("anyconnect")); 152​ d->ui.cmbProtocol->setCurrentIndex(dataMap[NM_OPENCONNECT_KEY_PROTOCOL] != QLatin1String("anyconnect"));
73 d->ui.leGateway->setText(dataMap[NM_OPENCONNECT_KEY_GATEWAY]); 153​ d->ui.leGateway->setText(dataMap[NM_OPENCONNECT_KEY_GATEWAY]);
Context not available.
78 d->ui.leUserCert->setUrl(QUrl::fromLocalFile(dataMap[NM_OPENCONNECT_KEY_USERCERT])); 158​ d->ui.leUserCert->setUrl(QUrl::fromLocalFile(dataMap[NM_OPENCONNECT_KEY_USERCERT]));
79 d->ui.leUserPrivateKey->setUrl(QUrl::fromLocalFile(dataMap[NM_OPENCONNECT_KEY_PRIVKEY])); 159​ d->ui.leUserPrivateKey->setUrl(QUrl::fromLocalFile(dataMap[NM_OPENCONNECT_KEY_PRIVKEY]));
80 d->ui.chkUseFsid->setChecked(dataMap[NM_OPENCONNECT_KEY_PEM_PASSPHRASE_FSID] == "yes"); 160​ d->ui.chkUseFsid->setChecked(dataMap[NM_OPENCONNECT_KEY_PEM_PASSPHRASE_FSID] == "yes");
161​ d->ui.preventInvalidCert->setChecked(dataMap[NM_OPENCONNECT_KEY_PREVENT_INVALID_CERT] == "yes");
162​
163​ int index = d->tokenModeList.indexOf(dataMap[NM_OPENCONNECT_KEY_TOKEN_MODE]);
pinoUnsubmitted
Not Done

const

pino: const
164​ if (index > 0) {
165​ d->tokenUi.cmbTokenMode->setCurrentIndex(index);
166​ if (index > 1) {
167​ loadSecrets(setting);
168​ }
169​ }
170​}
171​
172​void OpenconnectSettingWidget::loadSecrets(const NetworkManager::Setting::Ptr &setting)
173​{
174​ Q_D(OpenconnectSettingWidget);
175​
176​ NetworkManager::VpnSetting::Ptr vpnSetting = setting.staticCast<NetworkManager::VpnSetting>();
177​
178​ if (vpnSetting) {
179​ const NMStringMap secrets = vpnSetting->secrets();
180​ d->tokenUi.leTokenSecret->appendPlainText(secrets.value(NM_OPENCONNECT_KEY_TOKEN_SECRET));
181​ }
81} 182​}
82 183​
83QVariantMap OpenconnectSettingWidget::setting() const 184​QVariantMap OpenconnectSettingWidget::setting() const
Context not available.
88 setting.setServiceType(QLatin1String(NM_DBUS_SERVICE_OPENCONNECT)); 189​ setting.setServiceType(QLatin1String(NM_DBUS_SERVICE_OPENCONNECT));
89 190​
90 NMStringMap data; 191​ NMStringMap data;
192​ NMStringMap secrets;
91 193​
92 data.insert(NM_OPENCONNECT_KEY_PROTOCOL, d->ui.cmbProtocol->currentIndex() ? QLatin1String("nc") : QLatin1String("anyconnect")); 194​ 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()); 195​ 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()); 210​ data.insert(QLatin1String(NM_OPENCONNECT_KEY_PRIVKEY), d->ui.leUserPrivateKey->url().toLocalFile());
109 } 211​ }
110 data.insert(QLatin1String(NM_OPENCONNECT_KEY_PEM_PASSPHRASE_FSID), d->ui.chkUseFsid->isChecked() ? "yes" : "no"); 212​ data.insert(QLatin1String(NM_OPENCONNECT_KEY_PEM_PASSPHRASE_FSID), d->ui.chkUseFsid->isChecked() ? "yes" : "no");
213​ data.insert(QLatin1String(NM_OPENCONNECT_KEY_PREVENT_INVALID_CERT), d->ui.preventInvalidCert->isChecked() ? "yes" : "no");
111 214​
215​ data.insert(QLatin1String(NM_OPENCONNECT_KEY_TOKEN_MODE), d->tokenModeList[d->tokenUi.cmbTokenMode->currentIndex()]);
216​ secrets.insert(QLatin1String(NM_OPENCONNECT_KEY_TOKEN_SECRET), d->tokenUi.leTokenSecret->toPlainText());
217​
112 // Restore previous flags, this is necessary for keeping secrets stored in KWallet 218​ // Restore previous flags, this is necessary for keeping secrets stored in KWallet
113 Q_FOREACH (const QString &key, d->setting->data().keys()) { 219​ Q_FOREACH (const QString &key, d->setting->data().keys()) {
114 if (key.contains(QLatin1String("-flags"))) { 220​ if (key.contains(QLatin1String("-flags"))) {
Context not available.
122 data.insert(QLatin1String(NM_OPENCONNECT_KEY_GATEWAY"-flags"), QString::number(NetworkManager::Setting::NotSaved)); 228​ data.insert(QLatin1String(NM_OPENCONNECT_KEY_GATEWAY"-flags"), QString::number(NetworkManager::Setting::NotSaved));
123 229​
124 setting.setData(data); 230​ setting.setData(data);
125 setting.setSecrets(d->setting->secrets()); 231​ setting.setSecrets(secrets);
126 232​
127 return setting.toMap(); 233​ return setting.toMap();
128} 234​}
129 235​
236​void OpenconnectSettingWidget::showTokens()
237​{
238​ Q_D(OpenconnectSettingWidget);
239​
240​ d->tokenDlg->show();
241​}
pinoUnsubmitted
Not Done

missing empty line after this

pino: missing empty line after this
130bool OpenconnectSettingWidget::isValid() const 242​bool OpenconnectSettingWidget::isValid() const
131{ 243​{
132 Q_D(const OpenconnectSettingWidget); 244​ Q_D(const OpenconnectSettingWidget);
Context not available.
jgrulichUnsubmitted
Not Done

Also leftover from the other review.

jgrulich: Also leftover from the other review.