FdoSecrets: Implement unlock before search

Fixes #6942 and fixes #4443

- Return number of deleted entries
- Fix minor memory leak
- FdoSecrets: make all prompt truly async per spec and update tests
    * the waited signal may already be emitted before calling spy.wait(),
      causing the test to fail. This commit checks the count before waiting.
    * check unlock result after waiting for signal
- FdoSecrets: implement unlockBeforeSearch option
- FdoSecrets: make search always work regardless of entry group searching settings, fixes #6942
- FdoSecrets: cleanup gracefully even if some test failed
- FdoSecrets: make it safe to call prompts concurrently
- FdoSecrets: make sure in unit test we click on the correct dialog

Note on the unit tests: objects are not deleted (due to deleteLater event not handled).
So there may be multiple AccessControlDialog. But only one of
it is visible and is the correctly one to click on.

Before this change, a random one may be clicked on, causing the
completed signal never be sent.

share/translations/keepassxc_en.ts

@@ -7880,11 +7880,27 @@ Please consider generating a new key file.</source>
         <translation type="unfinished"></translation>
     </message>
     <message>
-        <source>&lt;html&gt;&lt;head/&gt;&lt;body&gt;&lt;p&gt;&lt;span style=&quot; font-family:&apos;-apple-system&apos;,&apos;BlinkMacSystemFont&apos;,&apos;Segoe UI&apos;,&apos;Helvetica&apos;,&apos;Arial&apos;,&apos;sans-serif&apos;,&apos;Apple Color Emoji&apos;,&apos;Segoe UI Emoji&apos;; font-size:14px; color:#24292e; background-color:#ffffff;&quot;&gt;This setting does not override disabling recycle bin prompts&lt;/span&gt;&lt;/p&gt;&lt;/body&gt;&lt;/html&gt;</source>
+        <source>Confirm when clients request entry deletion</source>
         <translation type="unfinished"></translation>
     </message>
     <message>
-        <source>Confirm when clients request entry deletion</source>
+        <source>&lt;html&gt;&lt;head/&gt;&lt;body&gt;&lt;p&gt;&lt;span style=&quot;
+           font-family:&apos;-apple-system&apos;,&apos;BlinkMacSystemFont&apos;,&apos;Segoe UI&apos;,&apos;Helvetica&apos;,&apos;Arial&apos;,&apos;sans-serif&apos;,&apos;Apple Color
+           Emoji&apos;,&apos;Segoe UI Emoji&apos;; font-size:14px; color:#24292e; background-color:#ffffff;&quot;&gt;This setting does
+           not override disabling recycle bin prompts&lt;/span&gt;&lt;/p&gt;&lt;/body&gt;&lt;/html&gt;
+          </source>
+        <translation type="unfinished"></translation>
+    </message>
+    <message>
+        <source>&lt;html&gt;&lt;head/&gt;&lt;body&gt;&lt;p&gt;This improves compatibility with certain applications
+           which search for password without unlocking the database first.&lt;/p&gt;&lt;p&gt;But enabling this may also
+           crash the client if the database can not be unlocked within a certain timeout. (Usually 25s, but may be a
+           different value set in applications.)&lt;/p&gt;&lt;/body&gt;&lt;/html&gt;
+          </source>
+        <translation type="unfinished"></translation>
+    </message>
+    <message>
+        <source>Prompt to unlock database before searching</source>
         <translation type="unfinished"></translation>
     </message>
 </context>

src/core/Config.cpp

@@ -178,6 +178,7 @@ static const QHash<Config::ConfigKey, ConfigDirective> configStrings = {
     {Config::FdoSecrets_ShowNotification, {QS("FdoSecrets/ShowNotification"), Roaming, true}},
     {Config::FdoSecrets_ConfirmDeleteItem, {QS("FdoSecrets/ConfirmDeleteItem"), Roaming, true}},
     {Config::FdoSecrets_ConfirmAccessItem, {QS("FdoSecrets/ConfirmAccessItem"), Roaming, true}},
+    {Config::FdoSecrets_UnlockBeforeSearch, {QS("FdoSecrets/UnlockBeforeSearch"), Roaming, true}},
 
     // KeeShare
     {Config::KeeShare_QuietSuccess, {QS("KeeShare/QuietSuccess"), Roaming, false}},

src/core/Config.h

@@ -155,6 +155,7 @@ class Config : public QObject
         FdoSecrets_ShowNotification,
         FdoSecrets_ConfirmDeleteItem,
         FdoSecrets_ConfirmAccessItem,
+        FdoSecrets_UnlockBeforeSearch,
 
         KeeShare_QuietSuccess,
         KeeShare_Own,

src/fdosecrets/FdoSecretsSettings.cpp

@@ -83,6 +83,16 @@ namespace FdoSecrets
         config()->set(Config::FdoSecrets_ConfirmAccessItem, confirmAccessItem);
     }
 
+    bool FdoSecretsSettings::unlockBeforeSearch() const
+    {
+        return config()->get(Config::FdoSecrets_UnlockBeforeSearch).toBool();
+    }
+
+    void FdoSecretsSettings::setUnlockBeforeSearch(bool unlockBeforeSearch)
+    {
+        config()->set(Config::FdoSecrets_UnlockBeforeSearch, unlockBeforeSearch);
+    }
+
     QUuid FdoSecretsSettings::exposedGroup(const QSharedPointer<Database>& db) const
     {
         return exposedGroup(db.data());

src/fdosecrets/FdoSecretsSettings.h

@@ -44,6 +44,9 @@ namespace FdoSecrets
         bool confirmAccessItem() const;
         void setConfirmAccessItem(bool confirmAccessItem);
 
+        bool unlockBeforeSearch() const;
+        void setUnlockBeforeSearch(bool unlockBeforeSearch);
+
         // Per db settings
 
         QUuid exposedGroup(const QSharedPointer<Database>& db) const;

src/fdosecrets/dbus/DBusMgr.cpp

@@ -177,14 +177,12 @@ namespace FdoSecrets
         // It's still weak and if the application does a prctl(PR_SET_DUMPABLE, 0) this link cannot be accessed.
         QFileInfo exe(QStringLiteral("/proc/%1/exe").arg(pid));
         info.exePath = exe.canonicalFilePath();
-        qDebug() << "process" << pid << info.exePath;
 
         // /proc/pid/cmdline gives full command line
         QFile cmdline(QStringLiteral("/proc/%1/cmdline").arg(pid));
         if (cmdline.open(QFile::ReadOnly)) {
             info.command = QString::fromLocal8Bit(cmdline.readAll().replace('\0', ' ')).trimmed();
         }
-        qDebug() << "process" << pid << info.command;
 
         // /proc/pid/stat gives ppid, name
         QFile stat(QStringLiteral("/proc/%1/stat").arg(pid));

src/fdosecrets/dbus/DBusObject.h

@@ -85,7 +85,7 @@ namespace FdoSecrets
 
         // Implicitly convert from QDBusError
         DBusResult(QDBusError::ErrorType error) // NOLINT(google-explicit-constructor)
-            : QString(QDBusError::errorString(error))
+            : QString(error == QDBusError::ErrorType::NoError ? "" : QDBusError::errorString(error))
         {
         }
 

src/fdosecrets/objects/Collection.cpp

@@ -24,7 +24,9 @@
 
 #include "core/Tools.h"
 #include "gui/DatabaseWidget.h"
+#include "gui/GuiTools.h"
 
+#include <QEventLoop>
 #include <QFileInfo>
 
 namespace FdoSecrets
@@ -62,9 +64,9 @@ namespace FdoSecrets
 
         // delete all items
         // this has to be done because the backend is actually still there, just we don't expose them
-        // NOTE: Do NOT use a for loop, because Item::doDelete will remove itself from m_items.
+        // NOTE: Do NOT use a for loop, because Item::removeFromDBus will remove itself from m_items.
         while (!m_items.isEmpty()) {
-            m_items.first()->doDelete();
+            m_items.first()->removeFromDBus();
         }
         cleanupConnections();
         dbus()->unregisterObject(this);
@@ -91,7 +93,7 @@ namespace FdoSecrets
     void Collection::reloadBackendOrDelete()
     {
         if (!reloadBackend()) {
-            doDelete();
+            removeFromDBus();
         }
     }
 
@@ -194,40 +196,49 @@ namespace FdoSecrets
         return {};
     }
 
-    DBusResult Collection::remove(const DBusClientPtr& client, PromptBase*& prompt)
+    DBusResult Collection::remove(PromptBase*& prompt)
     {
         auto ret = ensureBackend();
         if (ret.err()) {
             return ret;
         }
 
-        // Delete means close database
         prompt = PromptBase::Create<DeleteCollectionPrompt>(service(), this);
         if (!prompt) {
             return QDBusError::InternalError;
         }
-        if (backendLocked()) {
-            // this won't raise a dialog, immediate execute
-            ret = prompt->prompt(client, {});
-            if (ret.err()) {
-                return ret;
-            }
-            prompt = nullptr;
-        }
-        // defer the close to the prompt
         return {};
     }
 
-    DBusResult Collection::searchItems(const StringStringMap& attributes, QList<Item*>& items)
+    DBusResult
+    Collection::searchItems(const DBusClientPtr& client, const StringStringMap& attributes, QList<Item*>& items)
     {
         items.clear();
 
         auto ret = ensureBackend();
         if (ret.err()) {
             return ret;
         }
-        ret = ensureUnlocked();
-        if (ret.err()) {
+
+        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,
             // because there's no database at all when locked.
@@ -258,7 +269,11 @@ namespace FdoSecrets
             terms << attributeToTerm(it.key(), it.value());
         }
 
-        const auto foundEntries = EntrySearcher(false, true).search(terms, m_exposedGroup);
+        constexpr auto caseSensitive = false;
+        constexpr auto skipProtected = true;
+        constexpr auto forceSearch = true;
+        const auto foundEntries =
+            EntrySearcher(caseSensitive, skipProtected).search(terms, m_exposedGroup, forceSearch);
         items.reserve(foundEntries.size());
         for (const auto& entry : foundEntries) {
             items << m_entryToItem.value(entry);
@@ -298,36 +313,10 @@ namespace FdoSecrets
         if (ret.err()) {
             return ret;
         }
-        ret = ensureUnlocked();
-        if (ret.err()) {
-            return ret;
-        }
 
         item = nullptr;
-        QString itemPath;
-
-        auto iterAttr = properties.find(DBUS_INTERFACE_SECRET_ITEM + ".Attributes");
-        if (iterAttr != properties.end()) {
-            // the actual value in iterAttr.value() is QDBusArgument, which represents a structure
-            // and qt has no idea what this corresponds to.
-            // we thus force a conversion to StringStringMap here. The conversion is registered in
-            // DBusTypes.cpp
-            auto attributes = iterAttr.value().value<StringStringMap>();
-
-            itemPath = attributes.value(ItemAttributes::PathKey);
-
-            // check existing item using attributes
-            QList<Item*> existing;
-            ret = searchItems(attributes, existing);
-            if (ret.err()) {
-                return ret;
-            }
-            if (!existing.isEmpty() && replace) {
-                item = existing.front();
-            }
-        }
 
-        prompt = PromptBase::Create<CreateItemPrompt>(service(), this, properties, secret, itemPath, item);
+        prompt = PromptBase::Create<CreateItemPrompt>(service(), this, properties, secret, replace);
         if (!prompt) {
             return QDBusError::InternalError;
         }
@@ -418,7 +407,7 @@ namespace FdoSecrets
     void Collection::onDatabaseLockChanged()
     {
         if (!reloadBackend()) {
-            doDelete();
+            removeFromDBus();
             return;
         }
         emit collectionLockChanged(backendLocked());
@@ -436,7 +425,7 @@ namespace FdoSecrets
         auto newGroup = m_backend->database()->rootGroup()->findGroupByUuid(newUuid);
         if (!newGroup || inRecycleBin(newGroup)) {
             // no exposed group, delete self
-            doDelete();
+            removeFromDBus();
             return;
         }
 
@@ -505,7 +494,7 @@ namespace FdoSecrets
         // this has to be done because the backend is actually still there
         // just we don't expose them
         for (const auto& item : asConst(m_items)) {
-            item->doDelete();
+            item->removeFromDBus();
         }
 
         // repopulate
@@ -527,7 +516,7 @@ namespace FdoSecrets
         // forward delete signals
         connect(entry->group(), &Group::entryAboutToRemove, item, [item](Entry* toBeRemoved) {
             if (item->backend() == toBeRemoved) {
-                item->doDelete();
+                item->removeFromDBus();
             }
         });
 
@@ -568,17 +557,26 @@ namespace FdoSecrets
     {
         Q_ASSERT(m_backend);
 
-        return m_backend->lock();
+        // do not call m_backend->lock() directly
+        // let the service handle locking which handles concurrent calls
+        return service()->doLockDatabase(m_backend);
     }
 
     void Collection::doUnlock()
     {
         Q_ASSERT(m_backend);
 
-        service()->doUnlockDatabaseInDialog(m_backend);
+        return service()->doUnlockDatabaseInDialog(m_backend);
     }
 
-    void Collection::doDelete()
+    bool Collection::doDelete()
+    {
+        Q_ASSERT(m_backend);
+
+        return service()->doCloseDatabase(m_backend);
+    }
+
+    void Collection::removeFromDBus()
     {
         if (!m_backend) {
             // I'm already deleted
@@ -637,9 +635,18 @@ namespace FdoSecrets
         return !m_backend || !m_backend->database()->isInitialized() || m_backend->isLocked();
     }
 
-    void Collection::doDeleteEntries(QList<Entry*> entries)
+    bool Collection::doDeleteEntry(Entry* entry)
     {
-        m_backend->deleteEntries(std::move(entries), FdoSecrets::settings()->confirmDeleteItem());
+        // Confirm entry removal before moving forward
+        bool permanent = inRecycleBin(entry) || !m_backend->database()->metadata()->recycleBinEnabled();
+        if (FdoSecrets::settings()->confirmDeleteItem()
+            && !GuiTools::confirmDeleteEntries(m_backend, {entry}, permanent)) {
+            return false;
+        }
+
+        auto num = GuiTools::deleteEntriesResolveReferences(m_backend, {entry}, permanent);
+
+        return num != 0;
     }
 
     Group* Collection::findCreateGroupByPath(const QString& groupPath)

src/fdosecrets/objects/Collection.h

@@ -63,8 +63,10 @@ namespace FdoSecrets
 
         Q_INVOKABLE DBUS_PROPERTY DBusResult modified(qulonglong& modified) const;
 
-        Q_INVOKABLE DBusResult remove(const DBusClientPtr& client, PromptBase*& prompt);
-        Q_INVOKABLE DBusResult searchItems(const StringStringMap& attributes, QList<Item*>& items);
+        Q_INVOKABLE DBusResult remove(PromptBase*& prompt);
+        Q_INVOKABLE DBusResult searchItems(const DBusClientPtr& client,
+                                           const StringStringMap& attributes,
+                                           QList<Item*>& items);
         Q_INVOKABLE DBusResult
         createItem(const QVariantMap& properties, const Secret& secret, bool replace, Item*& item, PromptBase*& prompt);
 
@@ -112,14 +114,18 @@ namespace FdoSecrets
 
     public slots:
         // expose some methods for Prompt to use
+
         bool doLock();
+        // actually only closes database tab in KPXC
+        bool doDelete();
+        // Async, finish signal doneUnlockCollection
         void doUnlock();
+
+        bool doDeleteEntry(Entry* entry);
         Item* doNewItem(const DBusClientPtr& client, QString itemPath);
-        // will remove self
-        void doDelete();
 
-        // delete the Entry in backend from this collection
-        void doDeleteEntries(QList<Entry*> entries);
+        // Only delete from dbus, will remove self. Do not affect database in KPXC
+        void removeFromDBus();
 
         // force reload info from backend, potentially delete self
         bool reloadBackend();

src/fdosecrets/objects/Item.cpp

@@ -335,7 +335,14 @@ namespace FdoSecrets
         return m_backend;
     }
 
-    void Item::doDelete()
+    bool Item::doDelete()
+    {
+        Q_ASSERT(m_backend);
+
+        return collection()->doDeleteEntry(m_backend);
+    }
+
+    void Item::removeFromDBus()
     {
         emit itemAboutToDelete();
 

src/fdosecrets/objects/Item.h

@@ -91,8 +91,13 @@ namespace FdoSecrets
         QString path() const;
 
     public slots:
-        void doDelete();
+        // will actually delete the entry in KPXC
+        bool doDelete();
 
+        // Only delete from dbus, will remove self. Do not affect database in KPXC
+        void removeFromDBus();
+
+    private slots:
         /**
          * Check if the backend is a valid object, send error reply if not.
          * @return No error if the backend is valid.