Merge pull request #5660 from Aetf/fix/fdosecrets-5279

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));
+            m_secretService = Service::Create(this, m_dbTabs);
-            connect(m_secretService.data(), &Service::error, this, [this](const QString& msg) {
+            if (!m_secretService) {
-                emit error(tr("<b>Fdo Secret Service:</b> %1").arg(msg));
-            });
-            if (!m_secretService->initialize()) {
-                m_secretService.reset();
                 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 @@ signals:
 
 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,6 +17,7 @@
 
 #include "Collection.h"
 
+#include "fdosecrets/FdoSecretsPlugin.h"
 #include "fdosecrets/FdoSecretsSettings.h"
 #include "fdosecrets/objects/Item.h"
 #include "fdosecrets/objects/Prompt.h"
@@ -24,7 +25,6 @@
 #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::databaseFilePathChanged, this, &Collection::reloadBackendOrDelete);
-        connect(backend, &DatabaseWidget::databaseReplaced, this, &Collection::reloadBackend);
+        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();
-
-            unregisterCurrentPath();
-            m_registered = false;
-        }
-
         Q_ASSERT(m_backend);
 
-        // make sure we have updated copy of the filepath, which is used to identify the database.
+        // delete all items
-        m_backendPath = m_backend->database()->filePath();
+        // 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();
+        unregisterPrimaryPath();
 
-        // the database may not have a name (derived from filePath) yet, which may happen if it's newly created.
+        // make sure we have updated copy of the filepath, which is used to identify the database.
-        // defer the registration to next time a file path change happens.
+        m_backendPath = m_backend->database()->canonicalFilePath();
-        if (!name().isEmpty()) {
+
-            registerWithPath(
+        // register the object, handling potentially duplicated name
-                QStringLiteral(DBUS_PATH_TEMPLATE_COLLECTION).arg(p()->objectPath().path(), encodePath(name())),
+        auto name = encodePath(this->name());
-                new CollectionAdaptor(this));
+        auto path = QStringLiteral(DBUS_PATH_TEMPLATE_COLLECTION).arg(p()->objectPath().path(), name);
-            m_registered = true;
+        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);
+            auto existing = searchItems(attributes);
-            if (existings.isError()) {
+            if (existing.isError()) {
-                return existings;
+                return existing;
             }
-            if (!existings.value().isEmpty() && replace) {
+            if (!existing.value().isEmpty() && replace) {
-                item = existings.value().front();
+                item = existing.value().front();
                 newlyCreated = false;
             }
         }
@@ -400,11 +415,13 @@ namespace FdoSecrets
 
         emit aliasAboutToAdd(alias);
 
-        bool ok = QDBusConnection::sessionBus().registerObject(
+        bool ok =
-            QStringLiteral(DBUS_PATH_TEMPLATE_ALIAS).arg(p()->objectPath().path(), alias), this);
+            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 is a newly created db without saving to file,
-            // This name is also used to register dbus path.
+            // but we have to give a name, which is used to register dbus path.
-            // For simplicity, we don't monitor the name change.
+            // 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
+            // So the dbus object path is not updated if the db name changes.
-            // changes. This should not be a problem because once the database
+            // This should not be a problem because once the database gets saved,
-            // gets saved, the dbus path will be updated to use filename and
+            // the dbus path will be updated to use filename and everything back to normal.
-            // 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

src/fdosecrets/objects/DBusObject.cpp

@@ -17,31 +17,28 @@
 
 #include "DBusObject.h"
 
-#include <QDBusAbstractAdaptor>
 #include <QFile>
 #include <QRegularExpression>
 #include <QTextStream>
 #include <QUrl>
 #include <QUuid>
 
-#include <utility>
-
 namespace FdoSecrets
 {
 
     DBusObject::DBusObject(DBusObject* parent)
         : QObject(parent)
+        , m_dbusAdaptor(nullptr)
     {
     }
 
-    void DBusObject::registerWithPath(const QString& path, QDBusAbstractAdaptor* adaptor)
+    bool DBusObject::registerWithPath(const QString& path, bool primary)
     {
-        m_objectPath.setPath(path);
+        if (primary) {
-        m_dbusAdaptor = adaptor;
+            m_objectPath.setPath(path);
-        adaptor->setParent(this);
+        }
-        auto ok = QDBusConnection::sessionBus().registerObject(m_objectPath.path(), this);
+
-        Q_UNUSED(ok);
+        return QDBusConnection::sessionBus().registerObject(path, this);
-        Q_ASSERT(ok);
     }
 
     QString DBusObject::callingPeerName() const
@@ -57,7 +54,10 @@ namespace FdoSecrets
 
     QString encodePath(const QString& value)
     {
-        // force "-.~_" to be encoded
+        // toPercentEncoding encodes everything except those in the unreserved group:
+        // ALPHA / DIGIT / "-" / "." / "_" / "~"
+        // we want only ALPHA / DIGIT / "_", with "_" as the escape character
+        // so force "-.~_" to be encoded
         return QUrl::toPercentEncoding(value, "", "-.~_").replace('%', '_');
     }
 

src/fdosecrets/objects/DBusObject.h

@@ -21,6 +21,7 @@
 #include "fdosecrets/objects/DBusReturn.h"
 #include "fdosecrets/objects/DBusTypes.h"
 
+#include <QDBusAbstractAdaptor>
 #include <QDBusConnection>
 #include <QDBusConnectionInterface>
 #include <QDBusContext>
@@ -31,21 +32,19 @@
 #include <QObject>
 #include <QScopedPointer>
 
-class QDBusAbstractAdaptor;
-
 namespace FdoSecrets
 {
     class Service;
 
     /**
-     * @brief A common base class for all dbus-exposed objects
+     * @brief A common base class for all dbus-exposed objects.
+     * However, derived class should inherit from `DBusObjectHelper`, which is
+     * the only way to set DBus adaptor and enforces correct adaptor creation.
      */
     class DBusObject : public QObject, public QDBusContext
     {
         Q_OBJECT
     public:
-        explicit DBusObject(DBusObject* parent = nullptr);
-
         const QDBusObjectPath& objectPath() const
         {
             return m_objectPath;
@@ -57,12 +56,21 @@ namespace FdoSecrets
         }
 
     protected:
-        void registerWithPath(const QString& path, QDBusAbstractAdaptor* adaptor);
+        /**
+         * @brief Register this object at given DBus path
+         * @param path DBus path to register at
+         * @param primary whether this path to be considered primary. The primary path is the one to be returned by
+         * `DBusObject::objectPath`.
+         * @return true on success
+         */
+        bool registerWithPath(const QString& path, bool primary = true);
 
-        void unregisterCurrentPath()
+        void unregisterPrimaryPath()
         {
+            if (m_objectPath.path() == QStringLiteral("/")) {
+                return;
+            }
             QDBusConnection::sessionBus().unregisterObject(m_objectPath.path());
-            m_dbusAdaptor = nullptr;
             m_objectPath.setPath(QStringLiteral("/"));
         }
 
@@ -85,6 +93,8 @@ namespace FdoSecrets
         }
 
     private:
+        explicit DBusObject(DBusObject* parent);
+
         /**
          * Derived class should not directly use sendErrorReply.
          * Instead, use raiseError
@@ -92,12 +102,26 @@ namespace FdoSecrets
         using QDBusContext::sendErrorReply;
 
         template <typename U> friend class DBusReturn;
+        template <typename Object, typename Adaptor> friend class DBusObjectHelper;
 
-    private:
         QDBusAbstractAdaptor* m_dbusAdaptor;
         QDBusObjectPath m_objectPath;
     };
 
+    template <typename Object, typename Adaptor> class DBusObjectHelper : public DBusObject
+    {
+    protected:
+        explicit DBusObjectHelper(DBusObject* parent)
+            : DBusObject(parent)
+        {
+            // creating new Adaptor has to be delayed into constructor's body,
+            // and can't be simply moved to initializer list, because at that
+            // point the base QObject class hasn't been initialized and will sigfault.
+            m_dbusAdaptor = new Adaptor(static_cast<Object*>(this));
+            m_dbusAdaptor->setParent(this);
+        }
+    };
+
     /**
      * Return the object path of the pointed DBusObject, or "/" if the pointer is null
      * @tparam T

src/fdosecrets/objects/DBusTypes.h

@@ -43,6 +43,7 @@
 #define DBUS_PATH_TEMPLATE_SESSION "%1/session/%2"
 #define DBUS_PATH_TEMPLATE_COLLECTION "%1/collection/%2"
 #define DBUS_PATH_TEMPLATE_ITEM "%1/%2"
+#define DBUS_PATH_TEMPLATE_PROMPT "%1/prompt/%2"
 
 namespace FdoSecrets
 {

src/fdosecrets/objects/Item.cpp

@@ -27,10 +27,10 @@
 #include "core/EntryAttributes.h"
 #include "core/Group.h"
 #include "core/Metadata.h"
-#include "core/Tools.h"
 
 #include <QMimeDatabase>
 #include <QRegularExpression>
+#include <QScopedPointer>
 #include <QSet>
 #include <QTextCodec>
 
@@ -48,18 +48,36 @@ namespace FdoSecrets
         constexpr auto FDO_SECRETS_CONTENT_TYPE = "FDO_SECRETS_CONTENT_TYPE";
     } // namespace
 
+    Item* Item::Create(Collection* parent, Entry* backend)
+    {
+        QScopedPointer<Item> res{new Item(parent, backend)};
+
+        if (!res->registerSelf()) {
+            return nullptr;
+        }
+
+        return res.take();
+    }
+
     Item::Item(Collection* parent, Entry* backend)
-        : DBusObject(parent)
+        : DBusObjectHelper(parent)
         , m_backend(backend)
     {
         Q_ASSERT(!p()->objectPath().path().isEmpty());
 
-        registerWithPath(QStringLiteral(DBUS_PATH_TEMPLATE_ITEM).arg(p()->objectPath().path(), m_backend->uuidToHex()),
-                         new ItemAdaptor(this));
-
         connect(m_backend.data(), &Entry::entryModified, this, &Item::itemChanged);
     }
 
+    bool Item::registerSelf()
+    {
+        auto path = QStringLiteral(DBUS_PATH_TEMPLATE_ITEM).arg(p()->objectPath().path(), m_backend->uuidToHex());
+        bool ok = registerWithPath(path);
+        if (!ok) {
+            service()->plugin()->emitError(tr("Failed to register item on DBus at path '%1'").arg(path));
+        }
+        return ok;
+    }
+
     DBusReturn<bool> Item::locked() const
     {
         auto ret = ensureBackend();
@@ -206,8 +224,8 @@ namespace FdoSecrets
         if (ret.isError()) {
             return ret;
         }
-        auto prompt = new DeleteItemPrompt(service(), this);
+        auto prompt = DeleteItemPrompt::Create(service(), this);
-        return prompt;
+        return prompt.value();
     }
 
     DBusReturn<SecretStruct> Item::getSecret(Session* session)
@@ -322,7 +340,7 @@ namespace FdoSecrets
         // Unregister current path early, do not rely on deleteLater's call to destructor
         // as in case of Entry moving between groups, new Item will be created at the same DBus path
         // before the current Item is deleted in the event loop.
-        unregisterCurrentPath();
+        unregisterPrimaryPath();
 
         m_backend = nullptr;
         deleteLater();

src/fdosecrets/objects/Item.h

@@ -38,12 +38,23 @@ namespace FdoSecrets
     class Collection;
     class PromptBase;
 
-    class Item : public DBusObject
+    class Item : public DBusObjectHelper<Item, ItemAdaptor>
     {
         Q_OBJECT
-    public:
+
         explicit Item(Collection* parent, Entry* backend);
 
+    public:
+        /**
+         * @brief Create a new instance of `Item`.
+         * @param parent the owning `Collection`
+         * @param backend the `Entry` containing the data
+         * @return pointer to newly created Item, or nullptr if error
+         * This may be caused by
+         *   - DBus path registration error
+         */
+        static Item* Create(Collection* parent, Entry* backend);
+
         DBusReturn<bool> locked() const;
 
         DBusReturn<const StringStringMap> attributes() const;
@@ -90,7 +101,12 @@ namespace FdoSecrets
     public slots:
         void doDelete();
 
-    private:
+        /**
+         * @brief Register self on DBus
+         * @return
+         */
+        bool registerSelf();
+
         /**
          * Check if the backend is a valid object, send error reply if not.
          * @return No error if the backend is valid.

src/fdosecrets/objects/Prompt.cpp

@@ -17,6 +17,7 @@
 
 #include "Prompt.h"
 
+#include "fdosecrets/FdoSecretsPlugin.h"
 #include "fdosecrets/FdoSecretsSettings.h"
 #include "fdosecrets/objects/Collection.h"
 #include "fdosecrets/objects/Item.h"
@@ -33,15 +34,22 @@ namespace FdoSecrets
 {
 
     PromptBase::PromptBase(Service* parent)
-        : DBusObject(parent)
+        : DBusObjectHelper(parent)
     {
-        registerWithPath(
-            QStringLiteral("%1/prompt/%2").arg(p()->objectPath().path(), Tools::uuidToHex(QUuid::createUuid())),
-            new PromptAdaptor(this));
-
         connect(this, &PromptBase::completed, this, &PromptBase::deleteLater);
     }
 
+    bool PromptBase::registerSelf()
+    {
+        auto path = QStringLiteral(DBUS_PATH_TEMPLATE_PROMPT)
+                        .arg(p()->objectPath().path(), Tools::uuidToHex(QUuid::createUuid()));
+        bool ok = registerWithPath(path);
+        if (!ok) {
+            service()->plugin()->emitError(tr("Failed to register item on DBus at path '%1'").arg(path));
+        }
+        return ok;
+    }
+
     QWindow* PromptBase::findWindow(const QString& windowId)
     {
         // find parent window, or nullptr if not found
@@ -69,6 +77,15 @@ namespace FdoSecrets
         return {};
     }
 
+    DBusReturn<DeleteCollectionPrompt*> DeleteCollectionPrompt::Create(Service* parent, Collection* coll)
+    {
+        QScopedPointer<DeleteCollectionPrompt> res{new DeleteCollectionPrompt(parent, coll)};
+        if (!res->registerSelf()) {
+            return DBusReturn<>::Error(QDBusError::InvalidObjectPath);
+        }
+        return res.take();
+    }
+
     DeleteCollectionPrompt::DeleteCollectionPrompt(Service* parent, Collection* coll)
         : PromptBase(parent)
         , m_collection(coll)
@@ -100,6 +117,15 @@ namespace FdoSecrets
         return {};
     }
 
+    DBusReturn<CreateCollectionPrompt*> CreateCollectionPrompt::Create(Service* parent)
+    {
+        QScopedPointer<CreateCollectionPrompt> res{new CreateCollectionPrompt(parent)};
+        if (!res->registerSelf()) {
+            return DBusReturn<>::Error(QDBusError::InvalidObjectPath);
+        }
+        return res.take();
+    }
+
     CreateCollectionPrompt::CreateCollectionPrompt(Service* parent)
         : PromptBase(parent)
     {
@@ -136,6 +162,15 @@ namespace FdoSecrets
         return {};
     }
 
+    DBusReturn<LockCollectionsPrompt*> LockCollectionsPrompt::Create(Service* parent, const QList<Collection*>& colls)
+    {
+        QScopedPointer<LockCollectionsPrompt> res{new LockCollectionsPrompt(parent, colls)};
+        if (!res->registerSelf()) {
+            return DBusReturn<>::Error(QDBusError::InvalidObjectPath);
+        }
+        return res.take();
+    }
+
     LockCollectionsPrompt::LockCollectionsPrompt(Service* parent, const QList<Collection*>& colls)
         : PromptBase(parent)
     {
@@ -179,6 +214,16 @@ namespace FdoSecrets
         return {};
     }
 
+    DBusReturn<UnlockCollectionsPrompt*> UnlockCollectionsPrompt::Create(Service* parent,
+                                                                         const QList<Collection*>& coll)
+    {
+        QScopedPointer<UnlockCollectionsPrompt> res{new UnlockCollectionsPrompt(parent, coll)};
+        if (!res->registerSelf()) {
+            return DBusReturn<>::Error(QDBusError::InvalidObjectPath);
+        }
+        return res.take();
+    }
+
     UnlockCollectionsPrompt::UnlockCollectionsPrompt(Service* parent, const QList<Collection*>& colls)
         : PromptBase(parent)
     {
@@ -246,6 +291,15 @@ namespace FdoSecrets
         return {};
     }
 
+    DBusReturn<DeleteItemPrompt*> DeleteItemPrompt::Create(Service* parent, Item* item)
+    {
+        QScopedPointer<DeleteItemPrompt> res{new DeleteItemPrompt(parent, item)};
+        if (!res->registerSelf()) {
+            return DBusReturn<>::Error(QDBusError::InvalidObjectPath);
+        }
+        return res.take();
+    }
+
     DeleteItemPrompt::DeleteItemPrompt(Service* parent, Item* item)
         : PromptBase(parent)
         , m_item(item)

src/fdosecrets/objects/Prompt.h

@@ -32,12 +32,10 @@ namespace FdoSecrets
 
     class Service;
 
-    class PromptBase : public DBusObject
+    class PromptBase : public DBusObjectHelper<PromptBase, PromptAdaptor>
     {
         Q_OBJECT
     public:
-        explicit PromptBase(Service* parent);
-
         virtual DBusReturn<void> prompt(const QString& windowId) = 0;
 
         virtual DBusReturn<void> dismiss();
@@ -46,6 +44,9 @@ namespace FdoSecrets
         void completed(bool dismissed, const QVariant& result);
 
     protected:
+        explicit PromptBase(Service* parent);
+
+        bool registerSelf();
         QWindow* findWindow(const QString& windowId);
         Service* service() const;
     };
@@ -56,9 +57,11 @@ namespace FdoSecrets
     {
         Q_OBJECT
 
-    public:
         explicit DeleteCollectionPrompt(Service* parent, Collection* coll);
 
+    public:
+        static DBusReturn<DeleteCollectionPrompt*> Create(Service* parent, Collection* coll);
+
         DBusReturn<void> prompt(const QString& windowId) override;
 
     private:
@@ -69,9 +72,11 @@ namespace FdoSecrets
     {
         Q_OBJECT
 
-    public:
         explicit CreateCollectionPrompt(Service* parent);
 
+    public:
+        static DBusReturn<CreateCollectionPrompt*> Create(Service* parent);
+
         DBusReturn<void> prompt(const QString& windowId) override;
         DBusReturn<void> dismiss() override;
 
@@ -82,9 +87,12 @@ namespace FdoSecrets
     class LockCollectionsPrompt : public PromptBase
     {
         Q_OBJECT
-    public:
+
         explicit LockCollectionsPrompt(Service* parent, const QList<Collection*>& colls);
 
+    public:
+        static DBusReturn<LockCollectionsPrompt*> Create(Service* parent, const QList<Collection*>& colls);
+
         DBusReturn<void> prompt(const QString& windowId) override;
         DBusReturn<void> dismiss() override;
 
@@ -96,9 +104,12 @@ namespace FdoSecrets
     class UnlockCollectionsPrompt : public PromptBase
     {
         Q_OBJECT
-    public:
+
         explicit UnlockCollectionsPrompt(Service* parent, const QList<Collection*>& coll);
 
+    public:
+        static DBusReturn<UnlockCollectionsPrompt*> Create(Service* parent, const QList<Collection*>& coll);
+
         DBusReturn<void> prompt(const QString& windowId) override;
         DBusReturn<void> dismiss() override;
 
@@ -116,9 +127,11 @@ namespace FdoSecrets
     {
         Q_OBJECT
 
-    public:
         explicit DeleteItemPrompt(Service* parent, Item* item);
 
+    public:
+        static DBusReturn<DeleteItemPrompt*> Create(Service* parent, Item* item);
+
         DBusReturn<void> prompt(const QString& windowId) override;
 
     private:

src/fdosecrets/objects/Service.cpp

@@ -30,6 +30,7 @@
 #include <QDBusConnection>
 #include <QDBusServiceWatcher>
 #include <QDebug>
+#include <QSharedPointer>
 
 namespace
 {
@@ -38,13 +39,21 @@ namespace
 
 namespace FdoSecrets
 {
+    QSharedPointer<Service> Service::Create(FdoSecretsPlugin* plugin, QPointer<DatabaseTabWidget> dbTabs)
+    {
+        QSharedPointer<Service> res{new Service(plugin, std::move(dbTabs))};
+        if (!res->initialize()) {
+            return {};
+        }
+        return res;
+    }
 
     Service::Service(FdoSecretsPlugin* plugin,
                      QPointer<DatabaseTabWidget> dbTabs) // clazy: exclude=ctor-missing-parent-argument
-        : DBusObject(nullptr)
+        : DBusObjectHelper(nullptr)
         , m_plugin(plugin)
         , m_databases(std::move(dbTabs))
-        , m_insdieEnsureDefaultAlias(false)
+        , m_insideEnsureDefaultAlias(false)
         , m_serviceWatcher(nullptr)
     {
         connect(
@@ -59,19 +68,21 @@ namespace FdoSecrets
     bool Service::initialize()
     {
         if (!QDBusConnection::sessionBus().registerService(QStringLiteral(DBUS_SERVICE_SECRET))) {
-            emit error(tr("Failed to register DBus service at %1.<br/>").arg(QLatin1String(DBUS_SERVICE_SECRET))
+            plugin()->emitError(
-                       + m_plugin->reportExistingService());
+                tr("Failed to register DBus service at %1.<br/>").arg(QLatin1String(DBUS_SERVICE_SECRET))
+                + m_plugin->reportExistingService());
             return false;
         }
 
-        registerWithPath(QStringLiteral(DBUS_PATH_SECRETS), new ServiceAdaptor(this));
+        if (!registerWithPath(QStringLiteral(DBUS_PATH_SECRETS))) {
+            plugin()->emitError(tr("Failed to register DBus path %1.<br/>").arg(QStringLiteral(DBUS_PATH_SECRETS)));
+            return false;
+        }
 
         // Connect to service unregistered signal
         m_serviceWatcher.reset(new QDBusServiceWatcher());
-        connect(m_serviceWatcher.data(),
+        connect(
-                &QDBusServiceWatcher::serviceUnregistered,
+            m_serviceWatcher.get(), &QDBusServiceWatcher::serviceUnregistered, this, &Service::dbusServiceUnregistered);
-                this,
-                &Service::dbusServiceUnregistered);
 
         m_serviceWatcher->setConnection(QDBusConnection::sessionBus());
 
@@ -99,31 +110,32 @@ namespace FdoSecrets
         // When the Collection finds that no exposed group, it will delete itself.
         // Thus the service also needs to monitor it and recreate the collection if the user changes
         // from no exposed to exposed something.
+        connect(dbWidget, &DatabaseWidget::databaseReplaced, this, [this, dbWidget]() {
+            monitorDatabaseExposedGroup(dbWidget);
+        });
         if (!dbWidget->isLocked()) {
             monitorDatabaseExposedGroup(dbWidget);
         }
-        connect(dbWidget, &DatabaseWidget::databaseUnlocked, this, [this, dbWidget]() {
-            monitorDatabaseExposedGroup(dbWidget);
-        });
 
-        auto coll = new Collection(this, dbWidget);
+        auto coll = Collection::Create(this, dbWidget);
-        // Creation may fail if the database is not exposed.
+        if (!coll) {
-        // This is okay, because we monitor the expose settings above
-        if (!coll->isValid()) {
-            coll->deleteLater();
             return;
         }
 
         m_collections << coll;
         m_dbToCollection[dbWidget] = coll;
 
-        // handle alias
+        // keep record of the collection existence
+        connect(coll, &Collection::collectionAboutToDelete, this, [this, coll]() {
+            m_collections.removeAll(coll);
+            m_dbToCollection.remove(coll->backend());
+        });
+
+        // keep record of alias
         connect(coll, &Collection::aliasAboutToAdd, this, &Service::onCollectionAliasAboutToAdd);
         connect(coll, &Collection::aliasAdded, this, &Service::onCollectionAliasAdded);
         connect(coll, &Collection::aliasRemoved, this, &Service::onCollectionAliasRemoved);
 
-        ensureDefaultAlias();
-
         // Forward delete signal, we have to rely on filepath to identify the database being closed,
         // but we can not access m_backend safely because during the databaseClosed signal,
         // m_backend may already be reset to nullptr
@@ -138,14 +150,22 @@ namespace FdoSecrets
             }
         });
 
-        // relay signals
+        // actual load, must after updates to m_collections, because the reload may trigger
-        connect(coll, &Collection::collectionChanged, this, [this, coll]() { emit collectionChanged(coll); });
+        // another onDatabaseTabOpen, and m_collections will be used to prevent recursion.
-        connect(coll, &Collection::collectionAboutToDelete, this, [this, coll]() {
+        if (!coll->reloadBackend()) {
-            m_collections.removeAll(coll);
+            // error in dbus
-            m_dbToCollection.remove(coll->backend());
+            return;
-            emit collectionDeleted(coll);
+        }
-        });
+        if (!coll->backend()) {
+            // no exposed group on this db
+            return;
+        }
 
+        ensureDefaultAlias();
+
+        // only start relay signals when the collection is fully setup
+        connect(coll, &Collection::collectionChanged, this, [this, coll]() { emit collectionChanged(coll); });
+        connect(coll, &Collection::collectionAboutToDelete, this, [this, coll]() { emit collectionDeleted(coll); });
         if (emitSignal) {
             emit collectionCreated(coll);
         }
@@ -164,11 +184,11 @@ namespace FdoSecrets
 
     void Service::ensureDefaultAlias()
     {
-        if (m_insdieEnsureDefaultAlias) {
+        if (m_insideEnsureDefaultAlias) {
             return;
         }
 
-        m_insdieEnsureDefaultAlias = true;
+        m_insideEnsureDefaultAlias = true;
 
         auto coll = findCollection(m_databases->currentDatabaseWidget());
         if (coll) {
@@ -176,7 +196,7 @@ namespace FdoSecrets
             coll->addAlias(DEFAULT_ALIAS).okOrDie();
         }
 
-        m_insdieEnsureDefaultAlias = false;
+        m_insideEnsureDefaultAlias = false;
     }
 
     void Service::dbusServiceUnregistered(const QString& service)
@@ -184,8 +204,9 @@ namespace FdoSecrets
         Q_ASSERT(m_serviceWatcher);
 
         auto removed = m_serviceWatcher->removeWatchedService(service);
-        Q_UNUSED(removed);
+        if (!removed) {
-        Q_ASSERT(removed);
+            qDebug("FdoSecrets: Failed to remove service watcher");
+        }
 
         Session::CleanupNegotiation(service);
         auto sess = m_peerToSession.value(service, nullptr);
@@ -218,7 +239,10 @@ namespace FdoSecrets
         if (!ciphers) {
             return DBusReturn<>::Error(QDBusError::NotSupported);
         }
-        result = new Session(std::move(ciphers), callingPeerName(), this);
+        result = Session::Create(std::move(ciphers), callingPeerName(), this);
+        if (!result) {
+            return DBusReturn<>::Error(QDBusError::InvalidObjectPath);
+        }
 
         m_sessions.append(result);
         m_peerToSession[peer] = result;
@@ -240,17 +264,23 @@ namespace FdoSecrets
         // return existing collection if alias is non-empty and exists.
         auto collection = findCollection(alias);
         if (!collection) {
-            auto cp = new CreateCollectionPrompt(this);
+            auto cp = CreateCollectionPrompt::Create(this);
-            prompt = cp;
+            if (cp.isError()) {
+                return cp;
+            }
+            prompt = cp.value();
 
-            // collection will be created when the prompt complets.
+            // collection will be created when the prompt completes.
             // once it's done, we set additional properties on the collection
-            connect(cp, &CreateCollectionPrompt::collectionCreated, cp, [alias, properties](Collection* coll) {
+            connect(cp.value(),
-                coll->setProperties(properties).okOrDie();
+                    &CreateCollectionPrompt::collectionCreated,
-                if (!alias.isEmpty()) {
+                    cp.value(),
-                    coll->addAlias(alias).okOrDie();
+                    [alias, properties](Collection* coll) {
-                }
+                        coll->setProperties(properties).okOrDie();
-            });
+                        if (!alias.isEmpty()) {
+                            coll->addAlias(alias).okOrDie();
+                        }
+                    });
         }
         return collection;
     }
@@ -314,7 +344,11 @@ namespace FdoSecrets
             }
         }
         if (!toUnlock.isEmpty()) {
-            prompt = new UnlockCollectionsPrompt(this, toUnlock);
+            auto up = UnlockCollectionsPrompt::Create(this, toUnlock);
+            if (up.isError()) {
+                return up;
+            }
+            prompt = up.value();
         }
         return unlocked;
     }
@@ -352,7 +386,11 @@ namespace FdoSecrets
             }
         }
         if (!toLock.isEmpty()) {
-            prompt = new LockCollectionsPrompt(this, toLock);
+            auto lp = LockCollectionsPrompt::Create(this, toLock);
+            if (lp.isError()) {
+                return lp;
+            }
+            prompt = lp.value();
         }
         return locked;
     }

src/fdosecrets/objects/Service.h

@@ -24,9 +24,10 @@
 #include <QHash>
 #include <QObject>
 #include <QPointer>
-#include <QScopedPointer>
 #include <QVariant>
 
+#include <memory>
+
 class QDBusServiceWatcher;
 
 class DatabaseTabWidget;
@@ -44,14 +45,21 @@ namespace FdoSecrets
     class ServiceAdaptor;
     class Session;
 
-    class Service : public DBusObject // clazy: exclude=ctor-missing-parent-argument
+    class Service : public DBusObjectHelper<Service, ServiceAdaptor> // clazy: exclude=ctor-missing-parent-argument
     {
         Q_OBJECT
-    public:
-        explicit Service(FdoSecretsPlugin* plugin, QPointer<DatabaseTabWidget> dbTabs);
-        ~Service() override;
 
-        bool initialize();
+        explicit Service(FdoSecretsPlugin* plugin, QPointer<DatabaseTabWidget> dbTabs);
+
+    public:
+        /**
+         * @brief Create a new instance of `Service`. Its parent is set to `null`
+         * @return pointer to newly created Service, or nullptr if error
+         * This may be caused by
+         *   - failed initialization
+         */
+        static QSharedPointer<Service> Create(FdoSecretsPlugin* plugin, QPointer<DatabaseTabWidget> dbTabs);
+        ~Service() override;
 
         DBusReturn<QVariant> openSession(const QString& algorithm, const QVariant& input, Session*& result);
         DBusReturn<Collection*>
@@ -82,12 +90,6 @@ namespace FdoSecrets
         void sessionOpened(Session* sess);
         void sessionClosed(Session* sess);
 
-        /**
-         * Report error message to the GUI
-         * @param msg
-         */
-        void error(const QString& msg);
-
         /**
          * Finish signal for async action doUnlockDatabaseInDialog
          * @param accepted If false, the action is canceled by the user
@@ -131,6 +133,8 @@ namespace FdoSecrets
         void onCollectionAliasRemoved(const QString& alias);
 
     private:
+        bool initialize();
+
         /**
          * Find collection by alias name
          * @param alias
@@ -156,9 +160,9 @@ namespace FdoSecrets
         QList<Session*> m_sessions;
         QHash<QString, Session*> m_peerToSession;
 
-        bool m_insdieEnsureDefaultAlias;
+        bool m_insideEnsureDefaultAlias;
 
-        QScopedPointer<QDBusServiceWatcher> m_serviceWatcher;
+        std::unique_ptr<QDBusServiceWatcher> m_serviceWatcher;
     };
 
 } // namespace FdoSecrets

src/fdosecrets/objects/Session.cpp

@@ -16,6 +16,7 @@
  */
 #include "Session.h"
 
+#include "fdosecrets/FdoSecretsPlugin.h"
 #include "fdosecrets/objects/SessionCipher.h"
 
 #include "core/Tools.h"
@@ -23,21 +24,40 @@
 namespace FdoSecrets
 {
 
-    QHash<QString, QVariant> Session::negoniationState;
+    QHash<QString, QVariant> Session::negotiationState;
+
+    Session* Session::Create(std::unique_ptr<CipherPair>&& cipher, const QString& peer, Service* parent)
+    {
+        QScopedPointer<Session> res{new Session(std::move(cipher), peer, parent)};
+
+        if (!res->registerSelf()) {
+            return nullptr;
+        }
+
+        return res.take();
+    }
 
     Session::Session(std::unique_ptr<CipherPair>&& cipher, const QString& peer, Service* parent)
-        : DBusObject(parent)
+        : DBusObjectHelper(parent)
         , m_cipher(std::move(cipher))
         , m_peer(peer)
         , m_id(QUuid::createUuid())
     {
-        registerWithPath(QStringLiteral(DBUS_PATH_TEMPLATE_SESSION).arg(p()->objectPath().path(), id()),
+    }
-                         new SessionAdaptor(this));
+
+    bool Session::registerSelf()
+    {
+        auto path = QStringLiteral(DBUS_PATH_TEMPLATE_SESSION).arg(p()->objectPath().path(), id());
+        bool ok = registerWithPath(path);
+        if (!ok) {
+            service()->plugin()->emitError(tr("Failed to register session on DBus at path '%1'").arg(path));
+        }
+        return ok;
     }
 
     void Session::CleanupNegotiation(const QString& peer)
     {
-        negoniationState.remove(peer);
+        negotiationState.remove(peer);
     }
 
     DBusReturn<void> Session::close()
@@ -58,6 +78,11 @@ namespace FdoSecrets
         return Tools::uuidToHex(m_id);
     }
 
+    Service* Session::service() const
+    {
+        return qobject_cast<Service*>(parent());
+    }
+
     std::unique_ptr<CipherPair> Session::CreateCiphers(const QString& peer,
                                                        const QString& algorithm,
                                                        const QVariant& input,

src/fdosecrets/objects/Session.h

@@ -34,18 +34,30 @@ namespace FdoSecrets
 {
 
     class CipherPair;
-    class Session : public DBusObject
+    class Session : public DBusObjectHelper<Session, SessionAdaptor>
     {
         Q_OBJECT
+
+        explicit Session(std::unique_ptr<CipherPair>&& cipher, const QString& peer, Service* parent);
+
     public:
         static std::unique_ptr<CipherPair> CreateCiphers(const QString& peer,
                                                          const QString& algorithm,
-                                                         const QVariant& intpu,
+                                                         const QVariant& input,
                                                          QVariant& output,
                                                          bool& incomplete);
         static void CleanupNegotiation(const QString& peer);
 
-        explicit Session(std::unique_ptr<CipherPair>&& cipher, const QString& peer, Service* parent);
+        /**
+         * @brief Create a new instance of `Session`.
+         * @param cipher the negotiated cipher
+         * @param peer connecting peer
+         * @param parent the owning Service
+         * @return pointer to newly created Session, or nullptr if error
+         * This may be caused by
+         *   - DBus path registration error
+         */
+        static Session* Create(std::unique_ptr<CipherPair>&& cipher, const QString& peer, Service* parent);
 
         DBusReturn<void> close();
 
@@ -71,6 +83,8 @@ namespace FdoSecrets
 
         QString id() const;
 
+        Service* service() const;
+
     signals:
         /**
          * The session is going to be closed
@@ -78,12 +92,15 @@ namespace FdoSecrets
          */
         void aboutToClose();
 
+    private:
+        bool registerSelf();
+
     private:
         std::unique_ptr<CipherPair> m_cipher;
         QString m_peer;
         QUuid m_id;
 
-        static QHash<QString, QVariant> negoniationState;
+        static QHash<QString, QVariant> negotiationState;
     };
 
 } // namespace FdoSecrets

src/fdosecrets/objects/adaptors/CollectionAdaptor.cpp

@@ -27,12 +27,16 @@ namespace FdoSecrets
     CollectionAdaptor::CollectionAdaptor(Collection* parent)
         : DBusAdaptor(parent)
     {
-        connect(
+        // p() isn't ready yet as this is called in Parent's constructor
-            p(), &Collection::itemCreated, this, [this](const Item* item) { emit ItemCreated(objectPathSafe(item)); });
+        connect(parent, &Collection::itemCreated, this, [this](const Item* item) {
-        connect(
+            emit ItemCreated(objectPathSafe(item));
-            p(), &Collection::itemDeleted, this, [this](const Item* item) { emit ItemDeleted(objectPathSafe(item)); });
+        });
-        connect(
+        connect(parent, &Collection::itemDeleted, this, [this](const Item* item) {
-            p(), &Collection::itemChanged, this, [this](const Item* item) { emit ItemChanged(objectPathSafe(item)); });
+            emit ItemDeleted(objectPathSafe(item));
+        });
+        connect(parent, &Collection::itemChanged, this, [this](const Item* item) {
+            emit ItemChanged(objectPathSafe(item));
+        });
     }
 
     const QList<QDBusObjectPath> CollectionAdaptor::items() const

src/fdosecrets/objects/adaptors/DBusAdaptor.h

@@ -32,7 +32,7 @@ namespace FdoSecrets
     template <typename Parent> class DBusAdaptor : public QDBusAbstractAdaptor
     {
     public:
-        explicit DBusAdaptor(QObject* parent = nullptr)
+        explicit DBusAdaptor(Parent* parent = nullptr)
             : QDBusAbstractAdaptor(parent)
         {
         }

src/fdosecrets/objects/adaptors/PromptAdaptor.cpp

@@ -25,7 +25,8 @@ namespace FdoSecrets
     PromptAdaptor::PromptAdaptor(PromptBase* parent)
         : DBusAdaptor(parent)
     {
-        connect(p(), &PromptBase::completed, this, [this](bool dismissed, QVariant result) {
+        // p() isn't ready yet as this is called in Parent's constructor
+        connect(parent, &PromptBase::completed, this, [this](bool dismissed, QVariant result) {
             // make sure the result contains a valid value, otherwise QDBusVariant refuses to marshall it.
             if (!result.isValid()) {
                 result = QString{};

src/fdosecrets/objects/adaptors/ServiceAdaptor.cpp

@@ -29,13 +29,14 @@ namespace FdoSecrets
     ServiceAdaptor::ServiceAdaptor(Service* parent)
         : DBusAdaptor(parent)
     {
-        connect(p(), &Service::collectionCreated, this, [this](Collection* coll) {
+        // p() isn't ready yet as this is called in Parent's constructor
+        connect(parent, &Service::collectionCreated, this, [this](Collection* coll) {
             emit CollectionCreated(objectPathSafe(coll));
         });
-        connect(p(), &Service::collectionDeleted, this, [this](Collection* coll) {
+        connect(parent, &Service::collectionDeleted, this, [this](Collection* coll) {
             emit CollectionDeleted(objectPathSafe(coll));
         });
-        connect(p(), &Service::collectionChanged, this, [this](Collection* coll) {
+        connect(parent, &Service::collectionChanged, this, [this](Collection* coll) {
             emit CollectionChanged(objectPathSafe(coll));
         });
     }

tests/gui/TestGuiFdoSecrets.cpp

@@ -48,6 +48,7 @@
 #include <QLineEdit>
 #include <QPointer>
 #include <QSignalSpy>
+#include <QTemporaryDir>
 #include <QXmlStreamReader>
 
 #include <memory>
@@ -173,18 +174,10 @@ namespace
 
 using namespace FdoSecrets;
 
-// for use in QSignalSpy
-Q_DECLARE_METATYPE(Collection*);
-Q_DECLARE_METATYPE(Item*);
-
 TestGuiFdoSecrets::~TestGuiFdoSecrets() = default;
 
 void TestGuiFdoSecrets::initTestCase()
 {
-    // for use in QSignalSpy
-    qRegisterMetaType<Collection*>();
-    qRegisterMetaType<Item*>();
-
     QVERIFY(Crypto::init());
     Config::createTempFileInstance();
     config()->set(Config::AutoSaveAfterEveryChange, false);
@@ -267,7 +260,7 @@ void TestGuiFdoSecrets::init()
     m_dbWidget = m_tabWidget->currentDatabaseWidget();
     m_db = m_dbWidget->database();
 
-    // by default expsoe the root group
+    // by default expose the root group
     FdoSecrets::settings()->setExposedGroup(m_db, m_db->rootGroup()->uuid());
     QVERIFY(m_dbWidget->save());
 }
@@ -457,11 +450,11 @@ void TestGuiFdoSecrets::testServiceUnlock()
     auto coll = getDefaultCollection(service);
     QVERIFY(coll);
 
-    QSignalSpy spyCollectionCreated(service, SIGNAL(collectionCreated(Collection*)));
+    QSignalSpy spyCollectionCreated(&service->dbusAdaptor(), SIGNAL(CollectionCreated(QDBusObjectPath)));
     QVERIFY(spyCollectionCreated.isValid());
-    QSignalSpy spyCollectionDeleted(service, SIGNAL(collectionDeleted(Collection*)));
+    QSignalSpy spyCollectionDeleted(&service->dbusAdaptor(), SIGNAL(CollectionDeleted(QDBusObjectPath)));
     QVERIFY(spyCollectionDeleted.isValid());
-    QSignalSpy spyCollectionChanged(service, SIGNAL(collectionChanged(Collection*)));
+    QSignalSpy spyCollectionChanged(&service->dbusAdaptor(), SIGNAL(CollectionChanged(QDBusObjectPath)));
     QVERIFY(spyCollectionChanged.isValid());
 
     PromptBase* prompt = nullptr;
@@ -471,7 +464,7 @@ void TestGuiFdoSecrets::testServiceUnlock()
         QVERIFY(unlocked.isEmpty());
     }
     QVERIFY(prompt);
-    QSignalSpy spyPromptCompleted(prompt, SIGNAL(completed(bool, QVariant)));
+    QSignalSpy spyPromptCompleted(&prompt->dbusAdaptor(), SIGNAL(Completed(bool, QDBusVariant)));
     QVERIFY(spyPromptCompleted.isValid());
 
     // nothing is unlocked yet
@@ -509,14 +502,14 @@ void TestGuiFdoSecrets::testServiceUnlock()
         auto args = spyPromptCompleted.takeFirst();
         QCOMPARE(args.size(), 2);
         QCOMPARE(args.at(0).toBool(), false);
-        QCOMPARE(args.at(1).value<QList<QDBusObjectPath>>(), {coll->objectPath()});
+        QCOMPARE(args.at(1).value<QDBusVariant>().variant().value<QList<QDBusObjectPath>>(), {coll->objectPath()});
     }
     QCOMPARE(spyCollectionCreated.count(), 0);
     QCOMPARE(spyCollectionChanged.count(), 1);
     {
         auto args = spyCollectionChanged.takeFirst();
         QCOMPARE(args.size(), 1);
-        QCOMPARE(args.at(0).value<Collection*>(), coll.data());
+        QCOMPARE(args.at(0).value<QDBusObjectPath>(), coll->objectPath());
     }
     QCOMPARE(spyCollectionDeleted.count(), 0);
 }
@@ -528,11 +521,11 @@ void TestGuiFdoSecrets::testServiceLock()
     auto coll = getDefaultCollection(service);
     QVERIFY(coll);
 
-    QSignalSpy spyCollectionCreated(service, SIGNAL(collectionCreated(Collection*)));
+    QSignalSpy spyCollectionCreated(&service->dbusAdaptor(), SIGNAL(CollectionCreated(QDBusObjectPath)));
     QVERIFY(spyCollectionCreated.isValid());
-    QSignalSpy spyCollectionDeleted(service, SIGNAL(collectionDeleted(Collection*)));
+    QSignalSpy spyCollectionDeleted(&service->dbusAdaptor(), SIGNAL(CollectionDeleted(QDBusObjectPath)));
     QVERIFY(spyCollectionDeleted.isValid());
-    QSignalSpy spyCollectionChanged(service, SIGNAL(collectionChanged(Collection*)));
+    QSignalSpy spyCollectionChanged(&service->dbusAdaptor(), SIGNAL(CollectionChanged(QDBusObjectPath)));
     QVERIFY(spyCollectionChanged.isValid());
 
     // if the db is modified, prompt user
@@ -542,7 +535,7 @@ void TestGuiFdoSecrets::testServiceLock()
         CHECKED_DBUS_LOCAL_CALL(locked, service->lock({coll}, prompt));
         QCOMPARE(locked.size(), 0);
         QVERIFY(prompt);
-        QSignalSpy spyPromptCompleted(prompt, SIGNAL(completed(bool, QVariant)));
+        QSignalSpy spyPromptCompleted(&prompt->dbusAdaptor(), SIGNAL(Completed(bool, QDBusVariant)));
         QVERIFY(spyPromptCompleted.isValid());
 
         // prompt and click cancel
@@ -563,7 +556,7 @@ void TestGuiFdoSecrets::testServiceLock()
         CHECKED_DBUS_LOCAL_CALL(locked, service->lock({coll}, prompt));
         QCOMPARE(locked.size(), 0);
         QVERIFY(prompt);
-        QSignalSpy spyPromptCompleted(prompt, SIGNAL(completed(bool, QVariant)));
+        QSignalSpy spyPromptCompleted(&prompt->dbusAdaptor(), SIGNAL(Completed(bool, QDBusVariant)));
         QVERIFY(spyPromptCompleted.isValid());
 
         // prompt and click save
@@ -577,7 +570,7 @@ void TestGuiFdoSecrets::testServiceLock()
         auto args = spyPromptCompleted.takeFirst();
         QCOMPARE(args.count(), 2);
         QCOMPARE(args.at(0).toBool(), false);
-        QCOMPARE(args.at(1).value<QList<QDBusObjectPath>>(), {coll->objectPath()});
+        QCOMPARE(args.at(1).value<QDBusVariant>().variant().value<QList<QDBusObjectPath>>(), {coll->objectPath()});
     }
 
     QCOMPARE(spyCollectionCreated.count(), 0);
@@ -585,7 +578,7 @@ void TestGuiFdoSecrets::testServiceLock()
     {
         auto args = spyCollectionChanged.takeFirst();
         QCOMPARE(args.size(), 1);
-        QCOMPARE(args.at(0).value<Collection*>(), coll.data());
+        QCOMPARE(args.at(0).value<QDBusObjectPath>(), coll->objectPath());
     }
     QCOMPARE(spyCollectionDeleted.count(), 0);
 
@@ -641,7 +634,7 @@ void TestGuiFdoSecrets::testCollectionCreate()
     auto service = enableService();
     QVERIFY(service);
 
-    QSignalSpy spyCollectionCreated(service, SIGNAL(collectionCreated(Collection*)));
+    QSignalSpy spyCollectionCreated(&service->dbusAdaptor(), SIGNAL(CollectionCreated(QDBusObjectPath)));
     QVERIFY(spyCollectionCreated.isValid());
 
     // returns existing if alias is nonempty and exists
@@ -663,7 +656,7 @@ void TestGuiFdoSecrets::testCollectionCreate()
         QVERIFY(!created);
         QVERIFY(prompt);
 
-        QSignalSpy spyPromptCompleted(prompt, SIGNAL(completed(bool, QVariant)));
+        QSignalSpy spyPromptCompleted(&prompt->dbusAdaptor(), SIGNAL(Completed(bool, QDBusVariant)));
         QVERIFY(spyPromptCompleted.isValid());
 
         QTimer::singleShot(50, this, SLOT(createDatabaseCallback()));
@@ -674,7 +667,8 @@ void TestGuiFdoSecrets::testCollectionCreate()
         auto args = spyPromptCompleted.takeFirst();
         QCOMPARE(args.size(), 2);
         QCOMPARE(args.at(0).toBool(), false);
-        auto coll = FdoSecrets::pathToObject<Collection>(args.at(1).value<QDBusObjectPath>());
+        auto coll =
+            FdoSecrets::pathToObject<Collection>(args.at(1).value<QDBusVariant>().variant().value<QDBusObjectPath>());
         QVERIFY(coll);
 
         QCOMPARE(coll->backend()->database()->metadata()->name(), QStringLiteral("Test NewDB"));
@@ -683,7 +677,7 @@ void TestGuiFdoSecrets::testCollectionCreate()
         {
             args = spyCollectionCreated.takeFirst();
             QCOMPARE(args.size(), 1);
-            QCOMPARE(args.at(0).value<Collection*>(), coll);
+            QCOMPARE(args.at(0).value<QDBusObjectPath>(), coll->objectPath());
         }
     }
 }
@@ -722,18 +716,16 @@ void TestGuiFdoSecrets::testCollectionDelete()
     QVERIFY(service);
     auto coll = getDefaultCollection(service);
     QVERIFY(coll);
-    // closing the tab calls coll->deleteLater()
+    // save the path which will be gone after the deletion.
-    // but deleteLater is not processed in QApplication::processEvent
+    auto collPath = coll->objectPath();
-    // see https://doc.qt.io/qt-5/qcoreapplication.html#processEvents
-    auto rawColl = coll.data();
 
-    QSignalSpy spyCollectionDeleted(service, SIGNAL(collectionDeleted(Collection*)));
+    QSignalSpy spyCollectionDeleted(&service->dbusAdaptor(), SIGNAL(CollectionDeleted(QDBusObjectPath)));
     QVERIFY(spyCollectionDeleted.isValid());
 
     m_db->markAsModified();
     CHECKED_DBUS_LOCAL_CALL(prompt, coll->deleteCollection());
     QVERIFY(prompt);
-    QSignalSpy spyPromptCompleted(prompt, SIGNAL(completed(bool, QVariant)));
+    QSignalSpy spyPromptCompleted(&prompt->dbusAdaptor(), SIGNAL(Completed(bool, QDBusVariant)));
     QVERIFY(spyPromptCompleted.isValid());
 
     // prompt and click save
@@ -751,16 +743,60 @@ void TestGuiFdoSecrets::testCollectionDelete()
     auto args = spyPromptCompleted.takeFirst();
     QCOMPARE(args.count(), 2);
     QCOMPARE(args.at(0).toBool(), false);
-    QCOMPARE(args.at(1).toString(), QStringLiteral(""));
+    QCOMPARE(args.at(1).value<QDBusVariant>().variant().toString(), QStringLiteral(""));
 
     QCOMPARE(spyCollectionDeleted.count(), 1);
     {
         args = spyCollectionDeleted.takeFirst();
         QCOMPARE(args.size(), 1);
-        QCOMPARE(args.at(0).value<Collection*>(), rawColl);
+        QCOMPARE(args.at(0).value<QDBusObjectPath>(), collPath);
     }
 }
 
+void TestGuiFdoSecrets::testHiddenFilename()
+{
+    // when file name contains leading dot, all parts excepting the last should be used
+    // for collection name, and the registration should success
+    QVERIFY(m_dbFile->rename(QFileInfo(*m_dbFile).path() + "/.Name.kdbx"));
+
+    // reset is necessary to not hold database longer and cause connections
+    // not cleaned up when the database tab is closed.
+    m_db.reset();
+    QVERIFY(m_tabWidget->closeAllDatabaseTabs());
+    m_tabWidget->addDatabaseTab(m_dbFile->fileName(), false, "a");
+    m_dbWidget = m_tabWidget->currentDatabaseWidget();
+    m_db = m_dbWidget->database();
+
+    // enable the service
+    auto service = enableService();
+    QVERIFY(service);
+
+    // collection is properly registered
+    auto coll = getDefaultCollection(service);
+    QVERIFY(coll->objectPath().path() != "/");
+    QCOMPARE(coll->name(), QStringLiteral(".Name"));
+}
+
+void TestGuiFdoSecrets::testDuplicateName()
+{
+    QTemporaryDir dir;
+    QVERIFY(dir.isValid());
+    // create another file under different path but with the same filename
+    QString anotherFile = dir.path() + "/" + QFileInfo(*m_dbFile).fileName();
+    m_dbFile->copy(anotherFile);
+    m_tabWidget->addDatabaseTab(anotherFile, false, "a");
+
+    auto service = enableService();
+    QVERIFY(service);
+
+    // when two databases have the same name, one of it will have part of its uuid suffixed
+    const auto pathNoSuffix = QStringLiteral("/org/freedesktop/secrets/collection/KeePassXC");
+    CHECKED_DBUS_LOCAL_CALL(colls, service->collections());
+    QCOMPARE(colls.size(), 2);
+    QCOMPARE(colls[0]->objectPath().path(), pathNoSuffix);
+    QVERIFY(colls[1]->objectPath().path() != pathNoSuffix);
+}
+
 void TestGuiFdoSecrets::testItemCreate()
 {
     auto service = enableService();
@@ -770,6 +806,9 @@ void TestGuiFdoSecrets::testItemCreate()
     auto sess = openSession(service, DhIetf1024Sha256Aes128CbcPkcs7::Algorithm);
     QVERIFY(sess);
 
+    QSignalSpy spyItemCreated(&coll->dbusAdaptor(), SIGNAL(ItemCreated(QDBusObjectPath)));
+    QVERIFY(spyItemCreated.isValid());
+
     // create item
     StringStringMap attributes{
         {"application", "fdosecrets-test"},
@@ -779,6 +818,14 @@ void TestGuiFdoSecrets::testItemCreate()
     auto item = createItem(sess, coll, "abc", "Password", attributes, false);
     QVERIFY(item);
 
+    // signals
+    {
+        QCOMPARE(spyItemCreated.count(), 1);
+        auto args = spyItemCreated.takeFirst();
+        QCOMPARE(args.size(), 1);
+        QCOMPARE(args.at(0).value<QDBusObjectPath>(), item->objectPath());
+    }
+
     // attributes
     {
         CHECKED_DBUS_LOCAL_CALL(actual, item->attributes());
@@ -843,28 +890,56 @@ void TestGuiFdoSecrets::testItemReplace()
         QCOMPARE(unlocked.size(), 2);
     }
 
+    QSignalSpy spyItemCreated(&coll->dbusAdaptor(), SIGNAL(ItemCreated(QDBusObjectPath)));
+    QVERIFY(spyItemCreated.isValid());
+    QSignalSpy spyItemChanged(&coll->dbusAdaptor(), SIGNAL(ItemChanged(QDBusObjectPath)));
+    QVERIFY(spyItemChanged.isValid());
+
     {
         // when replace, existing item with matching attr is updated
         auto item3 = createItem(sess, coll, "abc3", "Password", attr2, true);
         QVERIFY(item3);
         QCOMPARE(item2, item3);
         COMPARE_DBUS_LOCAL_CALL(item3->label(), QStringLiteral("abc3"));
-        // there is still 2 entries
+        // there are still 2 entries
         QList<Item*> locked;
         CHECKED_DBUS_LOCAL_CALL(unlocked, service->searchItems({{"application", "fdosecrets-test"}}, locked));
         QCOMPARE(unlocked.size(), 2);
+
+        QCOMPARE(spyItemCreated.count(), 0);
+        // there may be multiple changed signals, due to each item attribute is set separately
+        QVERIFY(!spyItemChanged.isEmpty());
+        for (const auto& args : spyItemChanged) {
+            QCOMPARE(args.size(), 1);
+            QCOMPARE(args.at(0).value<QDBusObjectPath>(), item3->objectPath());
+        }
     }
 
+    spyItemCreated.clear();
+    spyItemChanged.clear();
     {
         // when NOT replace, another entry is created
         auto item4 = createItem(sess, coll, "abc4", "Password", attr2, false);
         QVERIFY(item4);
         COMPARE_DBUS_LOCAL_CALL(item2->label(), QStringLiteral("abc3"));
         COMPARE_DBUS_LOCAL_CALL(item4->label(), QStringLiteral("abc4"));
-        // there is 3 entries
+        // there are 3 entries
         QList<Item*> locked;
         CHECKED_DBUS_LOCAL_CALL(unlocked, service->searchItems({{"application", "fdosecrets-test"}}, locked));
         QCOMPARE(unlocked.size(), 3);
+
+        QCOMPARE(spyItemCreated.count(), 1);
+        {
+            auto args = spyItemCreated.takeFirst();
+            QCOMPARE(args.size(), 1);
+            QCOMPARE(args.at(0).value<QDBusObjectPath>(), item4->objectPath());
+        }
+        // there may be multiple changed signals, due to each item attribute is set separately
+        QVERIFY(!spyItemChanged.isEmpty());
+        for (const auto& args : spyItemChanged) {
+            QCOMPARE(args.size(), 1);
+            QCOMPARE(args.at(0).value<QDBusObjectPath>(), item4->objectPath());
+        }
     }
 }
 
@@ -949,15 +1024,16 @@ void TestGuiFdoSecrets::testItemDelete()
     QVERIFY(coll);
     auto item = getFirstItem(coll);
     QVERIFY(item);
-    auto rawItem = item.data();
+    // save the path which will be gone after the deletion.
+    auto itemPath = item->objectPath();
 
-    QSignalSpy spyItemDeleted(coll, SIGNAL(itemDeleted(Item*)));
+    QSignalSpy spyItemDeleted(&coll->dbusAdaptor(), SIGNAL(ItemDeleted(QDBusObjectPath)));
     QVERIFY(spyItemDeleted.isValid());
 
     CHECKED_DBUS_LOCAL_CALL(prompt, item->deleteItem());
     QVERIFY(prompt);
 
-    QSignalSpy spyPromptCompleted(prompt, SIGNAL(completed(bool, QVariant)));
+    QSignalSpy spyPromptCompleted(&prompt->dbusAdaptor(), SIGNAL(Completed(bool, QDBusVariant)));
     QVERIFY(spyPromptCompleted.isValid());
 
     // prompt and click save
@@ -980,7 +1056,7 @@ void TestGuiFdoSecrets::testItemDelete()
     {
         args = spyItemDeleted.takeFirst();
         QCOMPARE(args.size(), 1);
-        QCOMPARE(args.at(0).value<Item*>(), rawItem);
+        QCOMPARE(args.at(0).value<QDBusObjectPath>(), itemPath);
     }
 }
 
@@ -1047,7 +1123,7 @@ void TestGuiFdoSecrets::testExposeSubgroup()
     QCOMPARE(exposedEntries, subgroup->entries());
 }
 
-void TestGuiFdoSecrets::testModifiyingExposedGroup()
+void TestGuiFdoSecrets::testModifyingExposedGroup()
 {
     // test when exposed group is removed the collection is not exposed anymore
     auto subgroup = m_db->rootGroup()->findGroupByPath("/Homebanking");

tests/gui/TestGuiFdoSecrets.h

@@ -82,7 +82,10 @@ private slots:
     void testDefaultAliasAlwaysPresent();
 
     void testExposeSubgroup();
-    void testModifiyingExposedGroup();
+    void testModifyingExposedGroup();
+
+    void testHiddenFilename();
+    void testDuplicateName();
 
 protected slots:
     void createDatabaseCallback();