diff --git a/pglab/ConnectionConfigurationWidget.cpp b/pglab/ConnectionConfigurationWidget.cpp index 52f0e2b..6434cd8 100644 --- a/pglab/ConnectionConfigurationWidget.cpp +++ b/pglab/ConnectionConfigurationWidget.cpp @@ -117,10 +117,9 @@ ConnectionConfigurationWidget::ConnectionConfigurationWidget( SET_OBJECT_NAME(lblSsl); cmbbxSsl = new QComboBox; SET_OBJECT_NAME(cmbbxSsl); - cmbbxSsl->setModelColumn(0); + cmbbxSsl->setModelColumn(SslModeModel::Name); auto ssl_model = new SslModeModel(this); cmbbxSsl->setModel(ssl_model); - cmbbxSsl->setEditable(true); lblSsl->setBuddy(cmbbxSsl); lblCert = new QLabel; @@ -207,7 +206,13 @@ void ConnectionConfigurationWidget::setData(const ConnectionConfig &cfg) edtPassword->setText(""); cmbDbname->setCurrentText(cfg.dbname()); - cmbbxSsl->setCurrentText(cfg.getParameter("sslmode")); + auto sslMode = cfg.getParameter("sslmode"); + if (sslMode == "verify_ca") + sslMode = "verify-ca"; + else if (sslMode == "verify_full") + sslMode = "verify-full"; + auto sslIndex = cmbbxSsl->findData(sslMode); + cmbbxSsl->setCurrentIndex(sslIndex < 0 ? 0 : sslIndex); edtCert->setText(cfg.sslCert()); edtKey->setText(cfg.sslKey()); edtRootCert->setText(cfg.sslRootCert()); @@ -234,7 +239,7 @@ ConnectionConfig ConnectionConfigurationWidget::data() const cfg.setUser(edtUser->text()); cfg.setPassword(edtPassword->text()); cfg.setDbname(cmbDbname->currentText()); - cfg.setParameter("sslmode", cmbbxSsl->currentText()); + cfg.setParameter("sslmode", cmbbxSsl->currentData().toString()); cfg.setSslCert(edtCert->text()); cfg.setSslKey(edtKey->text()); cfg.setSslRootCert(edtRootCert->text()); diff --git a/pglab/SslModeModel.cpp b/pglab/SslModeModel.cpp index 9694d2f..2ed2e7a 100644 --- a/pglab/SslModeModel.cpp +++ b/pglab/SslModeModel.cpp @@ -1,12 +1,20 @@ #include "SslModeModel.h" +#include + +namespace { + +const QStringList sslModes = {QString(), "disable", "allow", "prefer", "require", "verify-ca", "verify-full"}; + +} + SslModeModel::SslModeModel(QObject *parent) : QAbstractListModel(parent) {} -int SslModeModel::rowCount(const QModelIndex &) const +int SslModeModel::rowCount(const QModelIndex &parent) const { - return 6; + return parent.isValid() ? 0 : static_cast(sslModes.size()); } int SslModeModel::columnCount(const QModelIndex &) const @@ -16,49 +24,43 @@ int SslModeModel::columnCount(const QModelIndex &) const QVariant SslModeModel::data(const QModelIndex &index, int role) const { + if (!index.isValid() || index.model() != this || index.row() < 0 || index.row() >= rowCount() + || index.column() < 0 || index.column() >= ColCount) + return {}; + + if (index.column() == Name) { + const auto &mode = sslModes.at(index.row()); + if (role == Qt::UserRole) + return mode; + if (role == Qt::DisplayRole || role == Qt::EditRole) + return index.row() == 0 ? tr("Default") : mode; + return {}; + } + QVariant v; if (role == Qt::DisplayRole) { switch (index.column()) { - case Name: - switch(index.row()) { - case 0: - v = tr("disable"); - break; - case 1: - v = tr("allow"); - break; - case 2: - v = tr("prefer"); - break; - case 3: - v = tr("require"); - break; - case 4: - v = tr("verify_ca"); - break; - case 5: - v = tr("verify_full"); - break; - } - break; case Description: switch(index.row()) { case 0: - v = tr("try a non encrypted connection only"); + v = tr("use the PostgreSQL default SSL mode"); break; case 1: - v = tr("try no encryption first then try encrypted"); + v = tr("try a non encrypted connection only"); break; case 2: - v = tr("try encrypted first then not encrypted"); + v = tr("try no encryption first then try encrypted"); break; case 3: - v = tr("require an encrypted connection"); + v = tr("try encrypted first then not encrypted"); break; case 4: - v = tr("verify encryption certificate has a valid signature"); + v = tr("require an encrypted connection"); break; case 5: + v = tr("verify encryption certificate has a valid signature"); + break; + case 6: v = tr("verify encryption certificate has a valid signature and matches the host"); break; } diff --git a/releasenotes/notes/fix-ssl-mode-selection-45cc41625d61af90.yaml b/releasenotes/notes/fix-ssl-mode-selection-45cc41625d61af90.yaml new file mode 100644 index 0000000..391022b --- /dev/null +++ b/releasenotes/notes/fix-ssl-mode-selection-45cc41625d61af90.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + Fixed SSL mode selection and saving in the connection configuration dialog. + The field now offers the supported modes, including the corrected verify-ca + and verify-full values, and a Default option to leave the mode unset. + Existing verify_ca and verify_full values are corrected when saved. diff --git a/tests/pglabtests/CMakeLists.txt b/tests/pglabtests/CMakeLists.txt index 4410a36..d74d768 100644 --- a/tests/pglabtests/CMakeLists.txt +++ b/tests/pglabtests/CMakeLists.txt @@ -21,10 +21,12 @@ add_executable(pglabtests tst_Range.cpp ResultSourceMappingTest.cpp tst_RecordPanel.cpp - ${CMAKE_SOURCE_DIR}/pglab/crud/RecordPanel.cpp + tst_SslModeModel.cpp + tst_ConnectionConfigurationWidget.cpp ) target_link_libraries(pglabtests PRIVATE + pglabapp pglablib pgsql core diff --git a/tests/pglabtests/tst_ConnectionConfigurationWidget.cpp b/tests/pglabtests/tst_ConnectionConfigurationWidget.cpp new file mode 100644 index 0000000..32eb0fd --- /dev/null +++ b/tests/pglabtests/tst_ConnectionConfigurationWidget.cpp @@ -0,0 +1,111 @@ +#include + +#include +#include +#include +#include +#include +#include + +#include "ConnectionConfig.h" +#include "ConnectionConfigurationWidget.h" +#include "ConnectionController.h" +#include "PrintTo_Qt.h" + +class ConnectionConfigurationWidgetTest : public ::testing::Test { +protected: + // SSL settings do not need the controller's database or connection tree initialized. + ConnectionController controller; +}; + +TEST_F(ConnectionConfigurationWidgetTest, SelectedSslModesRoundTripThroughTheWidget) +{ + ConnectionConfigurationWidget widget(&controller); + widget.setData(ConnectionConfig()); + auto combo = widget.findChild("cmbbxSsl"); + ASSERT_NE(combo, nullptr); + EXPECT_FALSE(combo->isEditable()); + + const QStringList modes = {"disable", "allow", "prefer", "require", "verify-ca", "verify-full"}; + for (const auto &mode : modes) { + SCOPED_TRACE(mode.toStdString()); + auto index = combo->findData(mode); + ASSERT_GT(index, 0); + combo->setCurrentIndex(index); + EXPECT_EQ(combo->currentText(), mode); + + auto saved = widget.data(); + EXPECT_EQ(saved.getParameter("sslmode"), mode); + ConnectionConfigurationWidget reopened(&controller); + reopened.setData(saved); + EXPECT_EQ(reopened.data().getParameter("sslmode"), mode); + EXPECT_EQ(reopened.findChild("cmbbxSsl")->currentText(), mode); + } + + combo->setCurrentIndex(combo->findData(QString())); + EXPECT_TRUE(widget.data().getParameter("sslmode").isEmpty()); + EXPECT_EQ(widget.data().getParameters().count("sslmode"), 0); +} + +TEST_F(ConnectionConfigurationWidgetTest, LegacyVerificationModesAreCanonicalized) +{ + ConnectionConfigurationWidget widget(&controller); + for (const auto &mode : {QStringLiteral("verify_ca"), QStringLiteral("verify_full")}) { + ConnectionConfig config; + config.setParameter("sslmode", mode); + widget.setData(config); + + auto canonical = QString(mode).replace('_', '-'); + EXPECT_EQ(widget.findChild("cmbbxSsl")->currentText(), canonical); + EXPECT_EQ(widget.data().getParameter("sslmode"), canonical); + EXPECT_EQ(config.getParameter("sslmode"), mode); + } +} + +TEST_F(ConnectionConfigurationWidgetTest, MissingOrUnknownModesResetToDefault) +{ + ConnectionConfigurationWidget widget(&controller); + for (const auto &mode : {QString(), QStringLiteral("unknown")}) { + ConnectionConfig config; + config.setParameter("sslmode", "require"); + widget.setData(config); + config.setParameter("sslmode", mode); + widget.setData(config); + + EXPECT_EQ(widget.findChild("cmbbxSsl")->currentText(), QStringLiteral("Default")); + EXPECT_TRUE(widget.data().getParameter("sslmode").isEmpty()); + } +} + +TEST_F(ConnectionConfigurationWidgetTest, DialogOnlySavesOnAccept) +{ + ConnectionConfig config; + config.setParameter("sslmode", "allow"); + for (bool accept : {false, true}) { + bool saved = false; + ConnectionConfigurationWidget::editExistingInWindow(&controller, config, + [&](ConnectionConfigurationWidget &widget) { + saved = true; + EXPECT_EQ(widget.data().getParameter("sslmode"), QStringLiteral("require")); + }); + + QDialog *dialog = nullptr; + for (auto window : QApplication::topLevelWidgets()) { + if (auto candidate = qobject_cast(window); + candidate && candidate->findChild()) { + dialog = candidate; + break; + } + } + ASSERT_NE(dialog, nullptr); + auto combo = dialog->findChild("cmbbxSsl"); + ASSERT_NE(combo, nullptr); + combo->setCurrentIndex(combo->findData(QStringLiteral("require"))); + auto buttons = dialog->findChild(); + ASSERT_NE(buttons, nullptr); + buttons->button(accept ? QDialogButtonBox::Ok : QDialogButtonBox::Cancel)->click(); + EXPECT_EQ(saved, accept); + EXPECT_EQ(config.getParameter("sslmode"), QStringLiteral("allow")); + QCoreApplication::sendPostedEvents(nullptr, QEvent::DeferredDelete); + } +} diff --git a/tests/pglabtests/tst_SslModeModel.cpp b/tests/pglabtests/tst_SslModeModel.cpp new file mode 100644 index 0000000..3a29944 --- /dev/null +++ b/tests/pglabtests/tst_SslModeModel.cpp @@ -0,0 +1,67 @@ +#include + +#include + +#include "ConnectionConfig.h" +#include "PrintTo_Qt.h" +#include "SslModeModel.h" + +TEST(SslModeModelTest, SelectedModesKeepTheirTextAndCanonicalValue) +{ + SslModeModel model; + QComboBox combo; + combo.setModel(&model); + + ASSERT_FALSE(combo.isEditable()); + const QStringList modes = {"disable", "allow", "prefer", "require", "verify-ca", "verify-full"}; + ASSERT_EQ(combo.count(), modes.size() + 1); + + for (const auto &mode : modes) { + SCOPED_TRACE(mode.toStdString()); + auto index = combo.findData(mode); + ASSERT_GT(index, 0); + combo.setCurrentIndex(index); + EXPECT_EQ(combo.currentText(), mode); + EXPECT_EQ(combo.currentData().toString(), mode); + + ConnectionConfig config; + config.setParameter("sslmode", combo.currentData().toString()); + EXPECT_EQ(config.getParameter("sslmode"), mode); + EXPECT_TRUE(config.connectionString().contains("sslmode=" + mode)); + + QComboBox reopened; + reopened.setModel(&model); + reopened.setCurrentIndex(reopened.findData(config.getParameter("sslmode"))); + EXPECT_EQ(reopened.currentText(), mode); + EXPECT_EQ(reopened.currentData().toString(), mode); + } +} + +TEST(SslModeModelTest, DefaultClearsAnExplicitMode) +{ + SslModeModel model; + QComboBox combo; + combo.setModel(&model); + + EXPECT_EQ(combo.currentText(), QStringLiteral("Default")); + EXPECT_EQ(combo.currentData().toString(), QString()); + EXPECT_EQ(combo.findData(QString()), 0); + + ConnectionConfig config; + config.setParameter("sslmode", "require"); + combo.setCurrentIndex(combo.findData(QStringLiteral("require"))); + combo.setCurrentIndex(combo.findData(QString())); + config.setParameter("sslmode", combo.currentData().toString()); + + EXPECT_TRUE(config.getParameter("sslmode").isEmpty()); + EXPECT_EQ(config.getParameters().count("sslmode"), 0); + EXPECT_FALSE(config.connectionString().contains("sslmode=")); +} + +TEST(SslModeModelTest, InvalidIndexesAndChildRowsAreEmpty) +{ + SslModeModel model; + EXPECT_FALSE(model.data(QModelIndex()).isValid()); + EXPECT_FALSE(model.data(model.index(model.rowCount(), 0)).isValid()); + EXPECT_EQ(model.rowCount(model.index(0, 0)), 0); +}