Secret Service: cleanup and fix crash

src/fdosecrets/FdoSecretsPlugin.cpp

@@ -26,8 +26,6 @@
 
 #include <QFile>
 
-#include <utility>
-
 using FdoSecrets::Service;
 
 // TODO: Only used for testing. Need to split service functions away from settings page.
@@ -65,12 +63,8 @@ void FdoSecretsPlugin::updateServiceState()
 {
     if (FdoSecrets::settings()->isEnabled()) {
         if (!m_secretService && m_dbTabs) {
-            m_secretService.reset(new Service(this, m_dbTabs));
-            connect(m_secretService.data(), &Service::error, this, [this](const QString& msg) {
-                emit error(tr("<b>Fdo Secret Service:</b> %1").arg(msg));
-            });
-            if (!m_secretService->initialize()) {
-                m_secretService.reset();
+            m_secretService = Service::Create(this, m_dbTabs);
+            if (!m_secretService) {
                 FdoSecrets::settings()->setEnabled(false);
                 return;
             }
@@ -107,6 +101,12 @@ void FdoSecretsPlugin::emitRequestShowNotification(const QString& msg, const QSt
     emit requestShowNotification(msg, title, 10000);
 }
 
+void FdoSecretsPlugin::emitError(const QString& msg)
+{
+    emit error(tr("<b>Fdo Secret Service:</b> %1").arg(msg));
+    qDebug() << msg;
+}
+
 QString FdoSecretsPlugin::reportExistingService() const
 {
     auto pidStr = tr("Unknown", "Unknown PID");

src/fdosecrets/FdoSecretsPlugin.h

@@ -22,7 +22,8 @@
 #include "gui/Icons.h"
 
 #include <QPointer>
-#include <QScopedPointer>
+
+#include <memory>
 
 class DatabaseTabWidget;
 
@@ -77,6 +78,12 @@ public slots:
     void emitRequestSwitchToDatabases();
     void emitRequestShowNotification(const QString& msg, const QString& title = {});
 
+    /**
+     * @brief Show error in the GUI
+     * @param msg
+     */
+    void emitError(const QString& msg);
+
 signals:
     void error(const QString& msg);
     void requestSwitchToDatabases();
@@ -86,7 +93,7 @@ public slots:
 
 private:
     QPointer<DatabaseTabWidget> m_dbTabs;
-    QScopedPointer<FdoSecrets::Service> m_secretService;
+    QSharedPointer<FdoSecrets::Service> m_secretService;
 };
 
 #endif // KEEPASSXC_FDOSECRETSPLUGIN_H

src/fdosecrets/objects/Collection.cpp

@@ -17,14 +17,14 @@
 
 #include "Collection.h"
 
+#include "fdosecrets/FdoSecretsPlugin.h"
 #include "fdosecrets/FdoSecretsSettings.h"
 #include "fdosecrets/objects/Item.h"
 #include "fdosecrets/objects/Prompt.h"
 #include "fdosecrets/objects/Service.h"
 #include "fdosecrets/objects/Session.h"
 
 #include "core/Config.h"
-#include "core/Database.h"
 #include "core/Tools.h"
 #include "gui/DatabaseTabWidget.h"
 #include "gui/DatabaseWidget.h"
@@ -34,16 +34,19 @@
 
 namespace FdoSecrets
 {
+    Collection* Collection::Create(Service* parent, DatabaseWidget* backend)
+    {
+        return new Collection(parent, backend);
+    }
 
     Collection::Collection(Service* parent, DatabaseWidget* backend)
-        : DBusObject(parent)
+        : DBusObjectHelper(parent)
         , m_backend(backend)
         , m_exposedGroup(nullptr)
-        , m_registered(false)
     {
         // whenever the file path or the database object itself change, we do a full reload.
-        connect(backend, &DatabaseWidget::databaseFilePathChanged, this, &Collection::reloadBackend);
-        connect(backend, &DatabaseWidget::databaseReplaced, this, &Collection::reloadBackend);
+        connect(backend, &DatabaseWidget::databaseFilePathChanged, this, &Collection::reloadBackendOrDelete);
+        connect(backend, &DatabaseWidget::databaseReplaced, this, &Collection::reloadBackendOrDelete);
 
         // also remember to clear/populate the database when lock state changes.
         connect(backend, &DatabaseWidget::databaseUnlocked, this, &Collection::onDatabaseLockChanged);
@@ -56,37 +59,36 @@ namespace FdoSecrets
             }
             emit doneUnlockCollection(accepted);
         });
-
-        reloadBackend();
     }
 
-    void Collection::reloadBackend()
+    bool Collection::reloadBackend()
     {
-        if (m_registered) {
-            // 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.
-            while (!m_items.isEmpty()) {
-                m_items.first()->doDelete();
-            }
-            cleanupConnections();
+        Q_ASSERT(m_backend);
 
-            unregisterCurrentPath();
-            m_registered = false;
+        // 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.
+        while (!m_items.isEmpty()) {
+            m_items.first()->doDelete();
         }
-
-        Q_ASSERT(m_backend);
+        cleanupConnections();
+        unregisterPrimaryPath();
 
         // make sure we have updated copy of the filepath, which is used to identify the database.
-        m_backendPath = m_backend->database()->filePath();
-
-        // the database may not have a name (derived from filePath) yet, which may happen if it's newly created.
-        // defer the registration to next time a file path change happens.
-        if (!name().isEmpty()) {
-            registerWithPath(
-                QStringLiteral(DBUS_PATH_TEMPLATE_COLLECTION).arg(p()->objectPath().path(), encodePath(name())),
-                new CollectionAdaptor(this));
-            m_registered = true;
+        m_backendPath = m_backend->database()->canonicalFilePath();
+
+        // register the object, handling potentially duplicated name
+        auto name = encodePath(this->name());
+        auto path = QStringLiteral(DBUS_PATH_TEMPLATE_COLLECTION).arg(p()->objectPath().path(), name);
+        if (!registerWithPath(path)) {
+            // try again with a suffix
+            name += QStringLiteral("_%1").arg(Tools::uuidToHex(QUuid::createUuid()).left(4));
+            path = QStringLiteral(DBUS_PATH_TEMPLATE_COLLECTION).arg(p()->objectPath().path(), name);
+
+            if (!registerWithPath(path)) {
+                service()->plugin()->emitError(tr("Failed to register database on DBus under the name '%1'").arg(name));
+                return false;
+            }
         }
 
         // populate contents after expose on dbus, because items rely on parent's dbus object path
@@ -95,6 +97,15 @@ namespace FdoSecrets
         } else {
             cleanupConnections();
         }
+
+        return true;
+    }
+
+    void Collection::reloadBackendOrDelete()
+    {
+        if (!reloadBackend()) {
+            doDelete();
+        }
     }
 
     DBusReturn<void> Collection::ensureBackend() const
@@ -197,7 +208,11 @@ namespace FdoSecrets
         }
 
         // Delete means close database
-        auto prompt = new DeleteCollectionPrompt(service(), this);
+        auto dpret = DeleteCollectionPrompt::Create(service(), this);
+        if (dpret.isError()) {
+            return dpret;
+        }
+        auto prompt = dpret.value();
         if (backendLocked()) {
             // this won't raise a dialog, immediate execute
             auto pret = prompt->prompt({});
@@ -311,12 +326,12 @@ namespace FdoSecrets
             itemPath = attributes.value(ItemAttributes::PathKey);
 
             // check existing item using attributes
-            auto existings = searchItems(attributes);
-            if (existings.isError()) {
-                return existings;
+            auto existing = searchItems(attributes);
+            if (existing.isError()) {
+                return existing;
             }
-            if (!existings.value().isEmpty() && replace) {
-                item = existings.value().front();
+            if (!existing.value().isEmpty() && replace) {
+                item = existing.value().front();
                 newlyCreated = false;
             }
         }
@@ -400,11 +415,13 @@ namespace FdoSecrets
 
         emit aliasAboutToAdd(alias);
 
-        bool ok = QDBusConnection::sessionBus().registerObject(
-            QStringLiteral(DBUS_PATH_TEMPLATE_ALIAS).arg(p()->objectPath().path(), alias), this);
+        bool ok =
+            registerWithPath(QStringLiteral(DBUS_PATH_TEMPLATE_ALIAS).arg(p()->objectPath().path(), alias), false);
         if (ok) {
             m_aliases.insert(alias);
             emit aliasAdded(alias);
+        } else {
+            return DBusReturn<>::Error(QDBusError::InvalidObjectPath);
         }
 
         return {};
@@ -435,16 +452,15 @@ namespace FdoSecrets
     QString Collection::name() const
     {
         if (m_backendPath.isEmpty()) {
-            // This is a newly created db without saving to file.
-            // This name is also used to register dbus path.
-            // For simplicity, we don't monitor the name change.
-            // So the dbus object path is not updated if the db name
-            // changes. This should not be a problem because once the database
-            // gets saved, the dbus path will be updated to use filename and
-            // everything back to normal.
+            // This is a newly created db without saving to file,
+            // but we have to give a name, which is used to register dbus path.
+            // We use database name for this purpose. For simplicity, we don't monitor the name change.
+            // So the dbus object path is not updated if the db name changes.
+            // This should not be a problem because once the database gets saved,
+            // the dbus path will be updated to use filename and everything back to normal.
             return m_backend->database()->metadata()->name();
         }
-        return QFileInfo(m_backendPath).baseName();
+        return QFileInfo(m_backendPath).completeBaseName();
     }
 
     DatabaseWidget* Collection::backend() const
@@ -466,7 +482,7 @@ namespace FdoSecrets
 
     void Collection::populateContents()
     {
-        if (!m_registered) {
+        if (!m_backend) {
             return;
         }
 
@@ -558,7 +574,7 @@ namespace FdoSecrets
             return;
         }
 
-        auto item = new Item(this, entry);
+        auto item = Item::Create(this, entry);
         m_items << item;
         m_entryToItem[entry] = item;
 
@@ -625,7 +641,7 @@ namespace FdoSecrets
 
         emit collectionAboutToDelete();
 
-        unregisterCurrentPath();
+        unregisterPrimaryPath();
 
         // remove alias manually to trigger signal
         for (const auto& a : aliases()) {
@@ -643,6 +659,8 @@ namespace FdoSecrets
         // reset backend and delete self
         m_backend = nullptr;
         deleteLater();
+
+        // items will be removed automatically as they are children objects
     }
 
     void Collection::cleanupConnections()

src/fdosecrets/objects/Collection.h

@@ -36,12 +36,24 @@ namespace FdoSecrets
     class Item;
     class PromptBase;
     class Service;
-    class Collection : public DBusObject
+    class Collection : public DBusObjectHelper<Collection, CollectionAdaptor>
     {
         Q_OBJECT
-    public:
+
         explicit Collection(Service* parent, DatabaseWidget* backend);
 
+    public:
+        /**
+         * @brief Create a new instance of `Collection`
+         * @param parent the owning Service
+         * @param backend the widget containing the database
+         * @return pointer to created instance, or nullptr when error happens.
+         * This may be caused by
+         *   - DBus path registration error
+         *   - database has no exposed group
+         */
+        static Collection* Create(Service* parent, DatabaseWidget* backend);
+
         DBusReturn<const QList<Item*>> items() const;
 
         DBusReturn<QString> label() const;
@@ -101,7 +113,7 @@ namespace FdoSecrets
         static EntrySearcher::SearchTerm attributeToTerm(const QString& key, const QString& value);
 
     public slots:
-        // expose some methods for Prmopt to use
+        // expose some methods for Prompt to use
         bool doLock();
         void doUnlock();
         // will remove self
@@ -110,11 +122,15 @@ namespace FdoSecrets
         // delete the Entry in backend from this collection
         void doDeleteEntries(QList<Entry*> entries);
 
+        // force reload info from backend, potentially delete self
+        bool reloadBackend();
+
     private slots:
         void onDatabaseLockChanged();
         void onDatabaseExposedGroupChanged();
-        // force reload info from backend, potentially delete self
-        void reloadBackend();
+
+        // calls reloadBackend, delete self when error
+        void reloadBackendOrDelete();
 
     private:
         friend class DeleteCollectionPrompt;
@@ -154,8 +170,6 @@ namespace FdoSecrets
         QSet<QString> m_aliases;
         QList<Item*> m_items;
         QMap<const Entry*, Item*> m_entryToItem;
-
-        bool m_registered;
     };
 
 } // namespace FdoSecrets