Fix bugs introduced by database refactor #2491 (#2503)

* Fix SSHAgent identity removal on database lock
* Refactor storage and manipulation of SSHAgent keys to streamline process with multiple db's
* Clear password field when widget is hidden, resolves #2502

src/gui/DatabaseOpenWidget.cpp

@@ -124,6 +124,12 @@ void DatabaseOpenWidget::hideEvent(QHideEvent* event)
     disconnect(YubiKey::instance(), nullptr, this, nullptr);
     m_yubiKeyBeingPolled = false;
 #endif
+
+    if (isVisible()) {
+        return;
+    }
+
+    clearForms();
 }
 
 void DatabaseOpenWidget::load(const QString& filename)
@@ -148,8 +154,9 @@ void DatabaseOpenWidget::load(const QString& filename)
 
 void DatabaseOpenWidget::clearForms()
 {
-    m_ui->editPassword->clear();
+    m_ui->editPassword->setText("");
     m_ui->comboKeyFile->clear();
+    m_ui->comboKeyFile->setEditText("");
     m_ui->checkPassword->setChecked(true);
     m_ui->checkKeyFile->setChecked(false);
     m_ui->checkChallengeResponse->setChecked(false);
@@ -225,7 +232,7 @@ void DatabaseOpenWidget::openDatabase()
     } else {
         m_ui->messageWidget->showMessage(tr("Unable to open the database:\n%1").arg(error),
                                          MessageWidget::Error);
-        m_ui->editPassword->clear();
+        m_ui->editPassword->setText("");
 
 #ifdef WITH_XC_TOUCHID
         // unable to unlock database, reset TouchID for the current database

src/gui/DatabaseWidget.cpp

@@ -212,11 +212,8 @@ DatabaseWidget::DatabaseWidget(QSharedPointer<Database> db, QWidget* parent)
 
 #ifdef WITH_XC_SSHAGENT
     if (config()->get("SSHAgent", false).toBool()) {
-        connect(this,
-                SIGNAL(currentModeChanged(DatabaseWidget::Mode)),
-                SSHAgent::instance(),
-                SLOT(databaseModeChanged(DatabaseWidget::Mode)));
-        connect(this, SIGNAL(closeRequest()), SSHAgent::instance(), SLOT(databaseModeChanged()));
+        connect(this, SIGNAL(databaseLocked()), SSHAgent::instance(), SLOT(databaseModeChanged()));
+        connect(this, SIGNAL(databaseUnlocked()), SSHAgent::instance(), SLOT(databaseModeChanged()));
     }
 #endif
 
@@ -1147,7 +1144,7 @@ bool DatabaseWidget::lock()
             if (!m_db->save(nullptr, false, false)) {
                 return false;
             }
-        } else if (isLocked()) {
+        } else if (!isLocked()) {
             QString msg;
             if (!m_db->metadata()->name().toHtmlEscaped().isEmpty()) {
                 msg = tr("\"%1\" was modified.\nSave changes?").arg(m_db->metadata()->name().toHtmlEscaped());

src/gui/PasswordEdit.cpp

@@ -61,7 +61,7 @@ void PasswordEdit::setShowPassword(bool show)
             setText(m_basePasswordEdit->text());
         } else {
             // This fix a bug when the QLineEdit is disabled while switching config
-            if (isEnabled() == false) {
+            if (!isEnabled()) {
                 setEnabled(true);
                 setReadOnly(false);
             }

src/gui/PasswordEdit.h

@@ -20,6 +20,7 @@
 #define KEEPASSX_PASSWORDEDIT_H
 
 #include <QLineEdit>
+#include <QPointer>
 
 class PasswordEdit : public QLineEdit
 {
@@ -46,7 +47,7 @@ private slots:
 private:
     bool passwordsEqual() const;
 
-    PasswordEdit* m_basePasswordEdit;
+    QPointer<PasswordEdit> m_basePasswordEdit;
 };
 
 #endif // KEEPASSX_PASSWORDEDIT_H

src/gui/entry/EditEntryWidget.cpp

@@ -1,5 +1,3 @@
-#include <utility>
-
 /*
  *  Copyright (C) 2010 Felix Geyer <debfx@fobos.de>
  *  Copyright (C) 2017 KeePassXC Team <team@keepassxc.org>
@@ -443,6 +441,8 @@ void EditEntryWidget::updateSSHAgentKeyInfo()
     if (SSHAgent::instance()->isAgentRunning()) {
         m_sshAgentUi->addToAgentButton->setEnabled(true);
         m_sshAgentUi->removeFromAgentButton->setEnabled(true);
+
+        SSHAgent::instance()->setAutoRemoveOnLock(key, m_sshAgentUi->removeKeyFromAgentCheckBox->isChecked());
     }
 }
 
@@ -558,21 +558,18 @@ void EditEntryWidget::addKeyToAgent()
     m_sshAgentUi->commentTextLabel->setText(key.comment());
     m_sshAgentUi->publicKeyEdit->document()->setPlainText(key.publicKey());
 
-    quint32 lifetime = 0;
+    int lifetime = 0;
     bool confirm = m_sshAgentUi->requireUserConfirmationCheckBox->isChecked();
 
     if (m_sshAgentUi->lifetimeCheckBox->isChecked()) {
         lifetime = m_sshAgentUi->lifetimeSpinBox->value();
     }
 
-    if (!SSHAgent::instance()->addIdentity(key, lifetime, confirm)) {
+    if (!SSHAgent::instance()->addIdentity(key, m_sshAgentUi->removeKeyFromAgentCheckBox->isChecked(),
+                                           static_cast<quint32>(lifetime), confirm)) {
         showMessage(SSHAgent::instance()->errorString(), MessageWidget::Error);
         return;
     }
-
-    if (m_sshAgentUi->removeKeyFromAgentCheckBox->isChecked()) {
-        SSHAgent::instance()->removeIdentityAtLock(key, m_entry->uuid());
-    }
 }
 
 void EditEntryWidget::removeKeyFromAgent()

src/gui/entry/EditEntryWidget.h

@@ -22,6 +22,7 @@
 #include <QModelIndex>
 #include <QScopedPointer>
 #include <QButtonGroup>
+#include <QPointer>
 
 #include "config-keepassx.h"
 #include "gui/EditWidget.h"

src/sshagent/SSHAgent.cpp

@@ -38,16 +38,17 @@ SSHAgent::SSHAgent(QObject* parent)
 
 SSHAgent::~SSHAgent()
 {
-    for (const QSet<OpenSSHKey>& keys : m_keys.values()) {
-        for (OpenSSHKey key : keys) {
+    auto it = m_addedKeys.begin();
+    while (it != m_addedKeys.end()) {
+        OpenSSHKey key = it.key();
         removeIdentity(key);
-        }
+        it = m_addedKeys.erase(it);
     }
 }
 
 SSHAgent* SSHAgent::instance()
 {
-    if (m_instance == nullptr) {
+    if (!m_instance) {
         qFatal("Race condition: instance wanted before it was initialized, this is a bug.");
     }
 
@@ -159,7 +160,16 @@ bool SSHAgent::sendMessage(const QByteArray& in, QByteArray& out)
 #endif
 }
 
-bool SSHAgent::addIdentity(OpenSSHKey& key, quint32 lifetime, bool confirm)
+/**
+ * Add the identity to the SSH agent.
+ *
+ * @param key identity / key to add
+ * @param lifetime time after which the key should expire
+ * @param confirm ask for confirmation before adding the key
+ * @param removeOnLock autoremove from agent when the Database is locked
+ * @return true on success
+ */
+bool SSHAgent::addIdentity(OpenSSHKey& key, bool removeOnLock, quint32 lifetime, bool confirm)
 {
     if (!isAgentRunning()) {
         m_error = tr("No agent running, cannot add identity.");
@@ -201,9 +211,18 @@ bool SSHAgent::addIdentity(OpenSSHKey& key, quint32 lifetime, bool confirm)
         return false;
     }
 
+    OpenSSHKey keyCopy = key;
+    keyCopy.clearPrivate();
+    m_addedKeys[keyCopy] = removeOnLock;
     return true;
 }
 
+/**
+ * Remove an identity from the SSH agent.
+ *
+ * @param key identity to remove
+ * @return true on success
+ */
 bool SSHAgent::removeIdentity(OpenSSHKey& key)
 {
     if (!isAgentRunning()) {
@@ -222,44 +241,49 @@ bool SSHAgent::removeIdentity(OpenSSHKey& key)
     request.writeString(keyData);
 
     QByteArray responseData;
-    if (!sendMessage(requestData, responseData)) {
-        return false;
+    return sendMessage(requestData, responseData);
 }
 
-    if (responseData.length() < 1 || static_cast<quint8>(responseData[0]) != SSH_AGENT_SUCCESS) {
-        m_error = tr("Agent does not have this identity.");
-        return false;
-    }
-
-    return true;
-}
-
-void SSHAgent::removeIdentityAtLock(const OpenSSHKey& key, const QUuid& uuid)
+/**
+ * Change "remove identity on lock" setting for a key already added to the agent.
+ * Will to nothing if the key has not been added to the agent.
+ *
+ * @param key key to change setting for
+ * @param autoRemove whether to remove the key from the agent when database is locked
+ */
+void SSHAgent::setAutoRemoveOnLock(const OpenSSHKey& key, bool autoRemove)
 {
-    OpenSSHKey copy = key;
-    copy.clearPrivate();
-    m_keys[uuid].insert(copy);
+    if (m_addedKeys.contains(key)) {
+        m_addedKeys[key] = autoRemove;
+    }
 }
 
-void SSHAgent::databaseModeChanged(DatabaseWidget::Mode mode)
+void SSHAgent::databaseModeChanged()
 {
-    DatabaseWidget* widget = qobject_cast<DatabaseWidget*>(sender());
-
-    if (widget == nullptr) {
+    auto* widget = qobject_cast<DatabaseWidget*>(sender());
+    if (!widget) {
         return;
     }
 
-    const QUuid& uuid = widget->database()->uuid();
-
-    if (mode == DatabaseWidget::Mode::LockedMode && m_keys.contains(uuid)) {
-
-        QSet<OpenSSHKey> keys = m_keys.take(uuid);
-        for (OpenSSHKey key : keys) {
+    if (widget->isLocked()) {
+        auto it = m_addedKeys.begin();
+        while (it != m_addedKeys.end()) {
+            OpenSSHKey key = it.key();
+            if (it.value()) {
                 if (!removeIdentity(key)) {
                     emit error(m_error);
                 }
+                it = m_addedKeys.erase(it);
+            } else {
+                // don't remove it yet
+                m_addedKeys[key] = false;
+                ++it;
             }
-    } else if (mode == DatabaseWidget::Mode::ViewMode && !m_keys.contains(uuid)) {
+        }
+
+        return;
+    }
+
     for (Entry* e : widget->database()->rootGroup()->entriesRecursive()) {
 
         if (widget->database()->metadata()->recycleBinEnabled()
@@ -322,21 +346,17 @@ void SSHAgent::databaseModeChanged(DatabaseWidget::Mode mode)
             key.setComment(fileName);
         }
 
-            if (settings.removeAtDatabaseClose()) {
-                removeIdentityAtLock(key, uuid);
-            }
-
-            if (settings.addAtDatabaseOpen()) {
-                int lifetime = 0;
+        if (!m_addedKeys.contains(key) && settings.addAtDatabaseOpen()) {
+            quint32 lifetime = 0;
 
             if (settings.useLifetimeConstraintWhenAdding()) {
-                    lifetime = settings.lifetimeConstraintDuration();
+                lifetime = static_cast<quint32>(settings.lifetimeConstraintDuration());
             }
 
-                if (!addIdentity(key, lifetime, settings.useConfirmConstraintWhenAdding())) {
+            if (!addIdentity(key, settings.removeAtDatabaseClose(),
+                lifetime, settings.useConfirmConstraintWhenAdding())) {
                 emit error(m_error);
             }
         }
     }
 }
-}

src/sshagent/SSHAgent.h

@@ -21,6 +21,7 @@
 
 #include "OpenSSHKey.h"
 #include <QList>
+#include <QHash>
 #include <QtCore>
 
 #include "gui/DatabaseWidget.h"
@@ -35,15 +36,15 @@ public:
 
     const QString errorString() const;
     bool isAgentRunning() const;
-    bool addIdentity(OpenSSHKey& key, quint32 lifetime = 0, bool confirm = false);
+    bool addIdentity(OpenSSHKey& key, bool removeOnLock, quint32 lifetime, bool confirm);
     bool removeIdentity(OpenSSHKey& key);
-    void removeIdentityAtLock(const OpenSSHKey& key, const QUuid& uuid);
+    void setAutoRemoveOnLock(const OpenSSHKey& key, bool autoRemove);
 
 signals:
     void error(const QString& message);
 
 public slots:
-    void databaseModeChanged(DatabaseWidget::Mode mode = DatabaseWidget::Mode::LockedMode);
+    void databaseModeChanged();
 
 private:
     const quint8 SSH_AGENT_FAILURE = 5;
@@ -71,7 +72,7 @@ private:
     const quint32 AGENT_COPYDATA_ID = 0x804e50ba;
 #endif
 
-    QMap<QUuid, QSet<OpenSSHKey>> m_keys;
+    QHash<OpenSSHKey, bool> m_addedKeys;
     QString m_error;
 };