Improve overall database settings behavior

* Fixes #10723 - only display password strength warning when actively editing the password * Also improve behavior of minimum quality warning * Improve behavior and handling of password changes with the database settings dialog * Prevents loss of newly entered password when toggling between elements in the settings page * On error, switch to tab that prevents saving database settings for easier correction
keepassxreboot · Jun 16, 2024 · 2c08448 · 2c08448
1 parent 2b08af7
commit 2c08448
Show file tree

Hide file tree

Showing 9 changed files with 93 additions and 85 deletions.
diff --git a/share/translations/keepassxc_en.ts b/share/translations/keepassxc_en.ts
@@ -1856,11 +1856,11 @@ Are you sure you want to continue without a password?</source>
         <translation type="unfinished"></translation>
     </message>
     <message>
-        <source>You must enter a stronger password to protect your database.</source>
+        <source>This is a weak password! For better protection of your secrets, you should choose a stronger password.</source>
         <translation type="unfinished"></translation>
     </message>
     <message>
-        <source>This is a weak password! For better protection of your secrets, you should choose a stronger password.</source>
+        <source>The provided password does not meet the minimum quality requirement.</source>
         <translation type="unfinished"></translation>
     </message>
 </context>

diff --git a/src/gui/databasekey/KeyComponentWidget.cpp b/src/gui/databasekey/KeyComponentWidget.cpp
@@ -73,6 +73,10 @@ KeyComponentWidget::Page KeyComponentWidget::visiblePage() const
 
 void KeyComponentWidget::updateAddStatus(bool added)
 {
+    if (m_ui->stackedWidget->currentIndex() == Page::Edit) {
+        emit editCanceled();
+    }
+
     if (added) {
         m_ui->stackedWidget->setCurrentIndex(Page::LeaveOrRemove);
     } else {
@@ -101,12 +105,6 @@ void KeyComponentWidget::cancelEdit()
     emit editCanceled();
 }
 
-void KeyComponentWidget::showEvent(QShowEvent* event)
-{
-    resetComponentEditWidget();
-    QWidget::showEvent(event);
-}
-
 void KeyComponentWidget::resetComponentEditWidget()
 {
     if (!m_componentWidget || static_cast<Page>(m_ui->stackedWidget->currentIndex()) == Page::Edit) {

diff --git a/src/gui/databasekey/KeyComponentWidget.h b/src/gui/databasekey/KeyComponentWidget.h
@@ -104,9 +104,6 @@ class KeyComponentWidget : public QWidget
     void editCanceled();
     void componentRemovalRequested();
 
-protected:
-    void showEvent(QShowEvent* event) override;
-
 private slots:
     void updateAddStatus(bool added);
     void doAdd();

diff --git a/src/gui/databasekey/PasswordEditWidget.cpp b/src/gui/databasekey/PasswordEditWidget.cpp
@@ -27,6 +27,9 @@ PasswordEditWidget::PasswordEditWidget(QWidget* parent)
     , m_compUi(new Ui::PasswordEditWidget())
 {
     initComponent();
+
+    // Explicitly clear password on cancel
+    connect(this, &PasswordEditWidget::editCanceled, this, [this] { setPassword({}); });
 }
 
 PasswordEditWidget::~PasswordEditWidget() = default;
@@ -59,7 +62,7 @@ bool PasswordEditWidget::isPasswordVisible() const
 
 bool PasswordEditWidget::isEmpty() const
 {
-    return (visiblePage() == Page::Edit) && m_compUi->enterPasswordEdit->text().isEmpty();
+    return visiblePage() != Page::Edit || m_compUi->enterPasswordEdit->text().isEmpty();
 }
 
 PasswordHealth::Quality PasswordEditWidget::getPasswordQuality() const
@@ -83,8 +86,7 @@ void PasswordEditWidget::initComponentEditWidget(QWidget* widget)
 {
     Q_UNUSED(widget);
     Q_ASSERT(m_compEditWidget);
-    m_compUi->enterPasswordEdit->setFocus();
-
+    setFocusProxy(m_compUi->enterPasswordEdit);
     m_compUi->enterPasswordEdit->setQualityVisible(true);
     m_compUi->repeatPasswordEdit->setQualityVisible(false);
 }
@@ -103,16 +105,6 @@ void PasswordEditWidget::initComponent()
            "<p>Good passwords are long and unique. KeePassXC can generate one for you.</p>"));
 }
 
-void PasswordEditWidget::hideEvent(QHideEvent* event)
-{
-    if (!isVisible() && m_compUi->enterPasswordEdit) {
-        m_compUi->enterPasswordEdit->setText("");
-        m_compUi->repeatPasswordEdit->setText("");
-    }
-
-    QWidget::hideEvent(event);
-}
-
 bool PasswordEditWidget::validate(QString& errorMessage) const
 {
     if (m_compUi->enterPasswordEdit->text() != m_compUi->repeatPasswordEdit->text()) {

diff --git a/src/gui/databasekey/PasswordEditWidget.h b/src/gui/databasekey/PasswordEditWidget.h
@@ -47,7 +47,6 @@ class PasswordEditWidget : public KeyComponentWidget
     QWidget* componentEditWidget() override;
     void initComponentEditWidget(QWidget* widget) override;
     void initComponent() override;
-    void hideEvent(QHideEvent* event) override;
 
 private slots:
     void setPassword(const QString& password);

diff --git a/src/gui/dbsettings/DatabaseSettingsDialog.cpp b/src/gui/dbsettings/DatabaseSettingsDialog.cpp
@@ -92,36 +92,32 @@ DatabaseSettingsDialog::DatabaseSettingsDialog(QWidget* parent)
     scrollArea->setSizeAdjustPolicy(QScrollArea::AdjustToContents);
     scrollArea->setWidgetResizable(true);
     scrollArea->setWidget(m_databaseKeyWidget);
-    m_securityTabWidget->addTab(scrollArea, tr("Database Credentials"));
 
+    m_securityTabWidget->addTab(scrollArea, tr("Database Credentials"));
     m_securityTabWidget->addTab(m_encryptionWidget, tr("Encryption Settings"));
+    m_securityTabWidget->setCurrentIndex(0);
 
     m_ui->categoryList->addCategory(tr("Remote Sync"), icons()->icon("remote-sync"));
     m_ui->stackedWidget->addWidget(m_remoteWidget);
 
-#if defined(WITH_XC_KEESHARE)
-    addSettingsPage(new DatabaseSettingsPageKeeShare());
+#ifdef WITH_XC_BROWSER
+    m_ui->categoryList->addCategory(tr("Browser Integration"), icons()->icon("internet-web-browser"));
+    m_ui->stackedWidget->addWidget(m_browserWidget);
 #endif
 
-#if defined(WITH_XC_FDOSECRETS)
-    addSettingsPage(new DatabaseSettingsPageFdoSecrets());
+#ifdef WITH_XC_KEESHARE
+    addSettingsPage(new DatabaseSettingsPageKeeShare());
 #endif
 
-    m_ui->stackedWidget->setCurrentIndex(0);
-    m_securityTabWidget->setCurrentIndex(0);
-
-    connect(m_securityTabWidget, SIGNAL(currentChanged(int)), SLOT(pageChanged()));
-    connect(m_ui->categoryList, SIGNAL(categoryChanged(int)), m_ui->stackedWidget, SLOT(setCurrentIndex(int)));
-
-#ifdef WITH_XC_BROWSER
-    m_ui->categoryList->addCategory(tr("Browser Integration"), icons()->icon("internet-web-browser"));
-    m_ui->stackedWidget->addWidget(m_browserWidget);
+#ifdef WITH_XC_FDOSECRETS
+    addSettingsPage(new DatabaseSettingsPageFdoSecrets());
 #endif
 
     m_ui->categoryList->addCategory(tr("Maintenance"), icons()->icon("hammer-wrench"));
     m_ui->stackedWidget->addWidget(m_maintenanceWidget);
 
-    pageChanged();
+    m_ui->stackedWidget->setCurrentIndex(0);
+    connect(m_ui->categoryList, SIGNAL(categoryChanged(int)), m_ui->stackedWidget, SLOT(setCurrentIndex(int)));
 }
 
 DatabaseSettingsDialog::~DatabaseSettingsDialog() = default;
@@ -171,21 +167,29 @@ void DatabaseSettingsDialog::showRemoteSettings()
 void DatabaseSettingsDialog::save()
 {
     if (!m_generalWidget->save()) {
+        m_ui->categoryList->setCurrentCategory(0);
         return;
     }
 
     if (!m_databaseKeyWidget->save()) {
+        m_ui->categoryList->setCurrentCategory(1);
+        m_securityTabWidget->setCurrentIndex(0);
         return;
     }
 
     if (!m_encryptionWidget->save()) {
+        m_ui->categoryList->setCurrentCategory(1);
+        m_securityTabWidget->setCurrentIndex(1);
         return;
     }
 
     if (!m_remoteWidget->save()) {
+        m_ui->categoryList->setCurrentCategory(2);
         return;
     }
 
+    // Browser settings don't have anything to save
+
     for (const ExtraPage& extraPage : asConst(m_extraPages)) {
         extraPage.saveSettings();
     }
@@ -195,10 +199,13 @@ void DatabaseSettingsDialog::save()
 
 void DatabaseSettingsDialog::reject()
 {
-    emit editFinished(false);
-}
+    m_generalWidget->discard();
+    m_databaseKeyWidget->discard();
+    m_encryptionWidget->discard();
+    m_remoteWidget->discard();
+#ifdef WITH_XC_BROWSER
+    m_browserWidget->discard();
+#endif
 
-void DatabaseSettingsDialog::pageChanged()
-{
-    m_ui->stackedWidget->currentIndex();
+    emit editFinished(false);
 }
diff --git a/src/gui/dbsettings/DatabaseSettingsDialog.h b/src/gui/dbsettings/DatabaseSettingsDialog.h
@@ -70,7 +70,6 @@ class DatabaseSettingsDialog : public DialogyWidget
 private slots:
     void save();
     void reject();
-    void pageChanged();
 
 private:
     enum Page

diff --git a/src/gui/dbsettings/DatabaseSettingsWidgetDatabaseKey.cpp b/src/gui/dbsettings/DatabaseSettingsWidgetDatabaseKey.cpp
@@ -79,28 +79,30 @@ void DatabaseSettingsWidgetDatabaseKey::load(QSharedPointer<Database> db)
         // database has no key, we are about to add a new one
         m_passwordEditWidget->changeVisiblePage(KeyComponentWidget::Page::Edit);
         m_passwordEditWidget->setPasswordVisible(true);
-    }
-
-    bool hasAdditionalKeys = false;
-    for (const auto& key : m_db->key()->keys()) {
-        if (key->uuid() == PasswordKey::UUID) {
-            m_passwordEditWidget->setComponentAdded(true);
-        } else if (key->uuid() == FileKey::UUID) {
-            m_keyFileEditWidget->setComponentAdded(true);
-            hasAdditionalKeys = true;
+        // Focus won't work until the UI settles
+        QTimer::singleShot(0, m_passwordEditWidget, SLOT(setFocus()));
+    } else {
+        bool hasAdditionalKeys = false;
+        for (const auto& key : m_db->key()->keys()) {
+            if (key->uuid() == PasswordKey::UUID) {
+                m_passwordEditWidget->setComponentAdded(true);
+            } else if (key->uuid() == FileKey::UUID) {
+                m_keyFileEditWidget->setComponentAdded(true);
+                hasAdditionalKeys = true;
+            }
         }
-    }
 
 #ifdef WITH_XC_YUBIKEY
-    for (const auto& key : m_db->key()->challengeResponseKeys()) {
-        if (key->uuid() == ChallengeResponseKey::UUID) {
-            m_yubiKeyEditWidget->setComponentAdded(true);
-            hasAdditionalKeys = true;
+        for (const auto& key : m_db->key()->challengeResponseKeys()) {
+            if (key->uuid() == ChallengeResponseKey::UUID) {
+                m_yubiKeyEditWidget->setComponentAdded(true);
+                hasAdditionalKeys = true;
+            }
         }
-    }
 #endif
 
-    setAdditionalKeyOptionsVisible(hasAdditionalKeys);
+        setAdditionalKeyOptionsVisible(hasAdditionalKeys);
+    }
 
     connect(m_passwordEditWidget->findChild<QPushButton*>("removeButton"), SIGNAL(clicked()), SLOT(markDirty()));
     connect(m_keyFileEditWidget->findChild<QPushButton*>("removeButton"), SIGNAL(clicked()), SLOT(markDirty()));
@@ -177,31 +179,32 @@ bool DatabaseSettingsWidgetDatabaseKey::save()
         return false;
     }
 
-    // Show warning if database password is weak
-    if (!m_passwordEditWidget->isEmpty()
-        && m_passwordEditWidget->getPasswordQuality() < PasswordHealth::Quality::Good) {
-        auto dialogResult = MessageBox::warning(this,
-                                                tr("Weak password"),
-                                                tr("This is a weak password! For better protection of your secrets, "
-                                                   "you should choose a stronger password."),
-                                                MessageBox::ContinueWithWeakPass | MessageBox::Cancel,
-                                                MessageBox::Cancel);
-
-        if (dialogResult == MessageBox::Cancel) {
+    if (!m_passwordEditWidget->isEmpty()) {
+        // Prevent setting password with a quality less than the minimum required
+        auto minQuality = qBound(0, config()->get(Config::Security_DatabasePasswordMinimumQuality).toInt(), 4);
+        if (m_passwordEditWidget->getPasswordQuality() < static_cast<PasswordHealth::Quality>(minQuality)) {
+            MessageBox::critical(this,
+                                 tr("Weak password"),
+                                 tr("The provided password does not meet the minimum quality requirement."),
+                                 MessageBox::Ok,
+                                 MessageBox::Ok);
             return false;
         }
-    }
 
-    // If enforced in the config file, deny users from continuing with a weak password
-    auto minQuality =
-        static_cast<PasswordHealth::Quality>(config()->get(Config::Security_DatabasePasswordMinimumQuality).toInt());
-    if (!m_passwordEditWidget->isEmpty() && m_passwordEditWidget->getPasswordQuality() < minQuality) {
-        MessageBox::critical(this,
-                             tr("Weak password"),
-                             tr("You must enter a stronger password to protect your database."),
-                             MessageBox::Ok,
-                             MessageBox::Ok);
-        return false;
+        // Show warning if database password is weak or poor
+        if (m_passwordEditWidget->getPasswordQuality() < PasswordHealth::Quality::Good) {
+            auto dialogResult =
+                MessageBox::warning(this,
+                                    tr("Weak password"),
+                                    tr("This is a weak password! For better protection of your secrets, "
+                                       "you should choose a stronger password."),
+                                    MessageBox::ContinueWithWeakPass | MessageBox::Cancel,
+                                    MessageBox::Cancel);
+
+            if (dialogResult == MessageBox::Cancel) {
+                return false;
+            }
+        }
     }
 
     if (!addToCompositeKey(m_keyFileEditWidget, newKey, oldFileKey)) {
@@ -232,11 +235,16 @@ bool DatabaseSettingsWidgetDatabaseKey::save()
         m_db->markAsModified();
     }
 
+    // Reset fields
+    initialize();
+
     return true;
 }
 
 void DatabaseSettingsWidgetDatabaseKey::discard()
 {
+    // Reset fields
+    initialize();
     emit editFinished(false);
 }
 

diff --git a/tests/gui/TestGui.cpp b/tests/gui/TestGui.cpp
@@ -1607,12 +1607,20 @@ void TestGui::testDatabaseSettings()
     passwordWidgets[0]->setText("b");
     passwordWidgets[1]->setText("b");
 
-    // Cancel password change
+    // Toggle between tabs to ensure the password remains
+    securityTabWidget->setCurrentIndex(1);
+    QApplication::processEvents();
+    securityTabWidget->setCurrentIndex(0);
+    QApplication::processEvents();
+    QCOMPARE(passwordWidgets[0]->text(), QString("b"));
+
+    // Cancel password change and confirm password is cleared
     auto cancelPasswordButton = passwordEditWidget->findChild<QPushButton*>("cancelButton");
     QVERIFY(cancelPasswordButton);
     QTest::mouseClick(cancelPasswordButton, Qt::LeftButton);
     QApplication::processEvents();
     QVERIFY(!passwordWidgets[0]->isVisible());
+    QCOMPARE(passwordWidgets[0]->text(), QString(""));
     QVERIFY(editPasswordButton->isVisible());
 
     // Switch to encryption tab and interact with various settings