FdoSecrets: be smarter about unlocking before search

The current logic is:
* If there're already unlocked collections, just use those,
* otherwise, show the unlock dialog until there's an unlocked and exposed collection.

Fix #7574

src/fdosecrets/objects/Collection.cpp

@@ -210,8 +210,7 @@ namespace FdoSecrets
         return {};
     }
 
-    DBusResult
-    Collection::searchItems(const DBusClientPtr& client, const StringStringMap& attributes, QList<Item*>& items)
+    DBusResult Collection::searchItems(const DBusClientPtr&, 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;
-        auto ret = collections(colls);
+        // we can only search unlocked collections
+        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()) {

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,
@@ -160,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{};

tests/gui/TestGuiFdoSecrets.cpp

@@ -182,7 +182,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
@@ -197,6 +197,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
@@ -387,6 +393,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();
@@ -1591,7 +1653,7 @@ void TestGuiFdoSecrets::testModifyingExposedGroup()
 
 void TestGuiFdoSecrets::lockDatabaseInBackend()
 {
-    m_dbWidget->lock();
+    m_tabWidget->lockDatabases();
     m_db.reset();
     processEvents();
 }
@@ -1789,14 +1851,16 @@ bool TestGuiFdoSecrets::driveNewDatabaseWizard()
     return ret;
 }
 
-bool TestGuiFdoSecrets::driveUnlockDialog()
+bool TestGuiFdoSecrets::driveUnlockDialog(DatabaseWidget* target)
 {
     processEvents();
     auto dbOpenDlg = m_tabWidget->findChild<DatabaseOpenDialog*>();
     VERIFY(dbOpenDlg);
     if (!dbOpenDlg->isVisible()) {
         return false;
     }
+    dbOpenDlg->setActiveDatabaseTab(target);
+
     auto editPassword = dbOpenDlg->findChild<QLineEdit*>("editPassword");
     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();
@@ -103,7 +104,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);