FdoSecrets: add smarter handling of database unlock requests

This commit implements the following logic:
* If there're already unlocked collections, just use those,
* otherwise, show the unlock dialog until there's an unlocked and exposed collection.

* Fixes #7574

src/fdosecrets/objects/Collection.cpp

@@ -210,8 +210,7 @@ namespace FdoSecrets
         return {};
     }
 
-    DBusResult
+    DBusResult Collection::searchItems(const DBusClientPtr&, const StringStringMap& attributes, QList<Item*>& items)
-    Collection::searchItems(const DBusClientPtr& client, const StringStringMap& attributes, QList<Item*>& items)
     {
         items.clear();
 
@@ -220,24 +219,6 @@ namespace FdoSecrets
             return ret;
         }
 
-        if (backendLocked() && settings()->unlockBeforeSearch()) {
-            // do a blocking unlock prompt first.
-            // while technically not correct, this should improve compatibility.
-            // see issue #4443
-            auto prompt = PromptBase::Create<UnlockPrompt>(service(), QSet<Collection*>{this}, QSet<Item*>{});
-            if (!prompt) {
-                return QDBusError::InternalError;
-            }
-            // we don't know the windowId to show the prompt correctly,
-            // but the default of showing over the KPXC main window should be good enough
-            ret = prompt->prompt(client, "");
-            // blocking wait
-            QEventLoop loop;
-            connect(prompt, &PromptBase::completed, &loop, &QEventLoop::quit);
-            loop.exec();
-        }
-
-        // check again because the prompt may be cancelled
         if (backendLocked()) {
             // searchItems should work, whether `this` is locked or not.
             // however, we can't search items the same way as in gnome-keying,

src/fdosecrets/objects/Service.cpp

@@ -184,6 +184,30 @@ namespace FdoSecrets
         return {};
     }
 
+    DBusResult Service::unlockedCollections(QList<Collection*>& unlocked) const
+    {
+        auto ret = collections(unlocked);
+        if (ret.err()) {
+            return ret;
+        }
+
+        // filter out locked collections
+        auto it = unlocked.begin();
+        while (it != unlocked.end()) {
+            bool isLocked = true;
+            ret = (*it)->locked(isLocked);
+            if (ret.err()) {
+                return ret;
+            }
+            if (isLocked) {
+                it = unlocked.erase(it);
+            } else {
+                ++it;
+            }
+        }
+        return {};
+    }
+
     DBusResult Service::openSession(const DBusClientPtr& client,
                                     const QString& algorithm,
                                     const QVariant& input,
@@ -242,15 +266,43 @@ namespace FdoSecrets
     DBusResult Service::searchItems(const DBusClientPtr& client,
                                     const StringStringMap& attributes,
                                     QList<Item*>& unlocked,
-                                    QList<Item*>& locked) const
+                                    QList<Item*>& locked)
     {
-        QList<Collection*> colls;
+        // we can only search unlocked collections
-        auto ret = collections(colls);
+        QList<Collection*> unlockedColls;
+        auto ret = unlockedCollections(unlockedColls);
         if (ret.err()) {
             return ret;
         }
 
-        for (const auto& coll : asConst(colls)) {
+        while (unlockedColls.isEmpty() && settings()->unlockBeforeSearch()) {
+            // enable compatibility mode by making sure at least one database is unlocked
+            QEventLoop loop;
+            bool wasAccepted = false;
+            connect(this, &Service::doneUnlockDatabaseInDialog, &loop, [&](bool accepted) {
+                wasAccepted = accepted;
+                loop.quit();
+            });
+
+            doUnlockAnyDatabaseInDialog();
+
+            // blocking wait
+            loop.exec();
+
+            if (!wasAccepted) {
+                // user cancelled, do not proceed
+                qWarning() << "user cancelled";
+                return {};
+            }
+
+            // need to recompute this because collections may disappear while in event loop
+            ret = unlockedCollections(unlockedColls);
+            if (ret.err()) {
+                return ret;
+            }
+        }
+
+        for (const auto& coll : asConst(unlockedColls)) {
             QList<Item*> items;
             ret = coll->searchItems(client, attributes, items);
             if (ret.err()) {
@@ -524,7 +576,7 @@ namespace FdoSecrets
         }
 
         // check if the db is already being unlocked to prevent multiple dialogs for the same db
-        if (m_unlockingDb.contains(dbWidget)) {
+        if (m_unlockingAnyDatabase || m_unlockingDb.contains(dbWidget)) {
             return;
         }
 
@@ -536,21 +588,33 @@ namespace FdoSecrets
         m_databases->unlockDatabaseInDialog(dbWidget, DatabaseOpenDialog::Intent::None);
     }
 
+    void Service::doUnlockAnyDatabaseInDialog()
+    {
+        if (m_unlockingAnyDatabase || !m_unlockingDb.isEmpty()) {
+            return;
+        }
+        m_unlockingAnyDatabase = true;
+
+        m_databases->unlockAnyDatabaseInDialog(DatabaseOpenDialog::Intent::None);
+    }
+
     void Service::onDatabaseUnlockDialogFinished(bool accepted, DatabaseWidget* dbWidget)
     {
-        if (!m_unlockingDb.contains(dbWidget)) {
+        if (!m_unlockingAnyDatabase && !m_unlockingDb.contains(dbWidget)) {
             // not our concern
             return;
         }
 
         if (!accepted) {
             emit doneUnlockDatabaseInDialog(false, dbWidget);
+            m_unlockingAnyDatabase = false;
             m_unlockingDb.remove(dbWidget);
         } else {
             // delay the done signal to when the database is actually done with unlocking
             // this is a oneshot connection to prevent superfluous signals
             auto conn = connect(dbWidget, &DatabaseWidget::databaseUnlocked, this, [dbWidget, this]() {
                 emit doneUnlockDatabaseInDialog(true, dbWidget);
+                m_unlockingAnyDatabase = false;
                 disconnect(m_unlockingDb.take(dbWidget));
             });
             m_unlockingDb[dbWidget] = conn;

src/fdosecrets/objects/Service.h

@@ -65,7 +65,7 @@ namespace FdoSecrets
         Q_INVOKABLE DBusResult searchItems(const DBusClientPtr& client,
                                            const StringStringMap& attributes,
                                            QList<Item*>& unlocked,
-                                           QList<Item*>& locked) const;
+                                           QList<Item*>& locked);
 
         Q_INVOKABLE DBusResult unlock(const DBusClientPtr& client,
                                       const QList<DBusObject*>& objects,
@@ -125,6 +125,12 @@ namespace FdoSecrets
          */
         void doUnlockDatabaseInDialog(DatabaseWidget* dbWidget);
 
+        /**
+         * Async, connect to signal doneUnlockDatabaseInDialog for finish notification
+         * @param dbWidget
+         */
+        void doUnlockAnyDatabaseInDialog();
+
     private slots:
         void ensureDefaultAlias();
 
@@ -154,6 +160,8 @@ namespace FdoSecrets
          */
         Collection* findCollection(const DatabaseWidget* db) const;
 
+        DBusResult unlockedCollections(QList<Collection*>& unlocked) const;
+
     private:
         FdoSecretsPlugin* m_plugin{nullptr};
         QPointer<DatabaseTabWidget> m_databases{};
@@ -165,6 +173,7 @@ namespace FdoSecrets
         QList<Session*> m_sessions{};
 
         bool m_insideEnsureDefaultAlias{false};
+        bool m_unlockingAnyDatabase{false};
         // list of db currently has unlock dialog shown
         QHash<const DatabaseWidget*, QMetaObject::Connection> m_unlockingDb{};
         QSet<const DatabaseWidget*> m_lockingDb{}; // list of db being locking

src/gui/DatabaseTabWidget.h

@@ -75,6 +75,7 @@ public slots:
     void closeDatabaseFromSender();
     void unlockDatabaseInDialog(DatabaseWidget* dbWidget, DatabaseOpenDialog::Intent intent);
     void unlockDatabaseInDialog(DatabaseWidget* dbWidget, DatabaseOpenDialog::Intent intent, const QString& filePath);
+    void unlockAnyDatabaseInDialog(DatabaseOpenDialog::Intent intent);
     void relockPendingDatabase();
 
     void showDatabaseSecurity();
@@ -106,7 +107,6 @@ private:
     QSharedPointer<Database> execNewDatabaseWizard();
     void updateLastDatabases(const QString& filename);
     bool warnOnExport();
-    void unlockAnyDatabaseInDialog(DatabaseOpenDialog::Intent intent);
     void displayUnlockDialog();
 
     QPointer<DatabaseWidgetStateSync> m_dbWidgetStateSync;

tests/gui/TestGuiFdoSecrets.cpp

@@ -183,7 +183,7 @@ void TestGuiFdoSecrets::init()
     m_dbFile.reset(new TemporaryFile());
     // Write the temp storage to a temp database file for use in our tests
     VERIFY(m_dbFile->open());
-    COMPARE(m_dbFile->write(m_dbData), static_cast<qint64>((m_dbData.size())));
+    COMPARE(m_dbFile->write(m_dbData), static_cast<qint64>(m_dbData.size()));
     m_dbFile->close();
 
     // make sure window is activated or focus tests may fail
@@ -198,6 +198,12 @@ void TestGuiFdoSecrets::init()
     // by default expose the root group
     FdoSecrets::settings()->setExposedGroup(m_db, m_db->rootGroup()->uuid());
     VERIFY(m_dbWidget->save());
+
+    // enforce consistent default settings at the beginning
+    FdoSecrets::settings()->setUnlockBeforeSearch(false);
+    FdoSecrets::settings()->setShowNotification(false);
+    FdoSecrets::settings()->setConfirmAccessItem(false);
+    FdoSecrets::settings()->setEnabled(false);
 }
 
 // Every test ends with closing the temp database without saving
@@ -388,6 +394,62 @@ void TestGuiFdoSecrets::testServiceSearchBlockingUnlock()
     }
 }
 
+void TestGuiFdoSecrets::testServiceSearchBlockingUnlockMultiple()
+{
+    // setup: two databases, both locked, one with exposed db, the other not.
+
+    // add another database tab with a database with no exposed group
+    // to avoid modify the original, copy to a temp file first
+    QFile sourceDbFile(QStringLiteral(KEEPASSX_TEST_DATA_DIR "/NewDatabase2.kdbx"));
+    QByteArray dbData;
+    VERIFY(sourceDbFile.open(QIODevice::ReadOnly));
+    VERIFY(Tools::readAllFromDevice(&sourceDbFile, dbData));
+    sourceDbFile.close();
+
+    QTemporaryFile anotherFile;
+    VERIFY(anotherFile.open());
+    COMPARE(anotherFile.write(dbData), static_cast<qint64>(dbData.size()));
+    anotherFile.close();
+
+    m_tabWidget->addDatabaseTab(anotherFile.fileName(), false);
+    auto anotherWidget = m_tabWidget->currentDatabaseWidget();
+
+    auto service = enableService();
+    VERIFY(service);
+
+    // when there are multiple locked databases,
+    // repeatly show the dialog until there is at least one unlocked collection
+    FdoSecrets::settings()->setUnlockBeforeSearch(true);
+
+    // when only unlocking the one with no exposed group, a second dialog is shown
+    lockDatabaseInBackend();
+    {
+        bool unlockDialogWorks = false;
+        QTimer::singleShot(50, [&]() {
+            unlockDialogWorks = driveUnlockDialog(anotherWidget);
+            QTimer::singleShot(50, [&]() { unlockDialogWorks &= driveUnlockDialog(); });
+        });
+
+        DBUS_GET2(unlocked, locked, service->SearchItems({{"Title", "Sample Entry"}}));
+        VERIFY(unlockDialogWorks);
+        COMPARE(locked, {});
+        COMPARE(unlocked.size(), 1);
+    }
+
+    // when unlocking the one with exposed group, the other one remains locked
+    lockDatabaseInBackend();
+    {
+        bool unlockDialogWorks = false;
+        QTimer::singleShot(50, [&]() { unlockDialogWorks = driveUnlockDialog(m_dbWidget); });
+
+        DBUS_GET2(unlocked, locked, service->SearchItems({{"Title", "Sample Entry"}}));
+        VERIFY(unlockDialogWorks);
+        COMPARE(locked, {});
+        COMPARE(unlocked.size(), 1);
+        VERIFY(anotherWidget->isLocked());
+    }
+}
+
 void TestGuiFdoSecrets::testServiceSearchForce()
 {
     auto service = enableService();
@@ -1625,7 +1687,7 @@ void TestGuiFdoSecrets::testNoExposeRecycleBin()
 
 void TestGuiFdoSecrets::lockDatabaseInBackend()
 {
-    m_dbWidget->lock();
+    m_tabWidget->lockDatabases();
     m_db.reset();
     processEvents();
 }
@@ -1825,7 +1887,7 @@ bool TestGuiFdoSecrets::driveNewDatabaseWizard()
     return ret;
 }
 
-bool TestGuiFdoSecrets::driveUnlockDialog()
+bool TestGuiFdoSecrets::driveUnlockDialog(DatabaseWidget* target)
 {
     processEvents();
     auto dbOpenDlg = m_tabWidget->findChild<DatabaseOpenDialog*>();
@@ -1833,6 +1895,8 @@ bool TestGuiFdoSecrets::driveUnlockDialog()
     if (!dbOpenDlg->isVisible()) {
         return false;
     }
+    dbOpenDlg->setActiveDatabaseTab(target);
+
     auto editPassword = dbOpenDlg->findChild<PasswordWidget*>("editPassword")->findChild<QLineEdit*>("passwordEdit");
     VERIFY(editPassword);
     editPassword->setFocus();

tests/gui/TestGuiFdoSecrets.h

@@ -67,6 +67,7 @@ private slots:
     void testServiceEnableNoExposedDatabase();
     void testServiceSearch();
     void testServiceSearchBlockingUnlock();
+    void testServiceSearchBlockingUnlockMultiple();
     void testServiceSearchForce();
     void testServiceUnlock();
     void testServiceUnlockDatabaseConcurrent();
@@ -104,7 +105,7 @@ private slots:
     void testDuplicateName();
 
 private:
-    bool driveUnlockDialog();
+    bool driveUnlockDialog(DatabaseWidget* target = nullptr);
     bool driveNewDatabaseWizard();
     bool driveAccessControlDialog(bool remember = true, bool includeFutureEntries = false);
     bool waitForSignal(QSignalSpy& spy, int expectedCount);