FdoSecrets: refactor DBus registration error handling

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;
             }
@@ -86,7 +80,7 @@ void FdoSecretsPlugin::updateServiceState()
 
 Service* FdoSecretsPlugin::serviceInstance() const
 {
-    return m_secretService.data();
+    return m_secretService.get();
 }
 
 DatabaseTabWidget* FdoSecretsPlugin::dbTabs() const
@@ -107,6 +101,11 @@ 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));
+}
+
 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;
+    std::unique_ptr<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"
@@ -34,6 +35,18 @@
 
 namespace FdoSecrets
 {
+    Collection* Collection::Create(Service* parent, DatabaseWidget* backend)
+    {
+        std::unique_ptr<Collection> coll{new Collection(parent, backend)};
+        if (!coll->reloadBackend()) {
+            return nullptr;
+        }
+        if (!coll->backend()) {
+            // no exposed group on this db
+            return nullptr;
+        }
+        return coll.release();
+    }
 
     Collection::Collection(Service* parent, DatabaseWidget* backend)
         : DBusObject(parent)
@@ -56,11 +69,9 @@ namespace FdoSecrets
             }
             emit doneUnlockCollection(accepted);
         });
-
-        reloadBackend();
     }
 
-    void Collection::reloadBackend()
+    bool Collection::reloadBackend()
     {
         if (m_registered) {
             // delete all items
@@ -83,9 +94,11 @@ namespace FdoSecrets
         // 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));
+            auto path = QStringLiteral(DBUS_PATH_TEMPLATE_COLLECTION).arg(p()->objectPath().path(), encodePath(name()));
+            if (!registerWithPath(path, new CollectionAdaptor(this))) {
+                service()->plugin()->emitError(tr("Failed to register database on DBus under the name '%1'").arg(name()));
+                return false;
+            }
             m_registered = true;
         }
 
@@ -95,6 +108,8 @@ namespace FdoSecrets
         } else {
             cleanupConnections();
         }
+
+        return true;
     }
 
     DBusReturn<void> Collection::ensureBackend() const
@@ -197,7 +212,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({});
@@ -405,6 +424,8 @@ namespace FdoSecrets
         if (ok) {
             m_aliases.insert(alias);
             emit aliasAdded(alias);
+        } else {
+            return DBusReturn<>::Error(QDBusError::InvalidObjectPath);
         }
 
         return {};
@@ -558,7 +579,7 @@ namespace FdoSecrets
             return;
         }
 
-        auto item = new Item(this, entry);
+        auto item = Item::Create(this, entry);
         m_items << item;
         m_entryToItem[entry] = item;
 

src/fdosecrets/objects/Collection.h

@@ -39,8 +39,19 @@ namespace FdoSecrets
     class Collection : public DBusObject
     {
         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;
 
@@ -101,7 +112,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
@@ -114,7 +125,7 @@ namespace FdoSecrets
         void onDatabaseLockChanged();
         void onDatabaseExposedGroupChanged();
         // force reload info from backend, potentially delete self
-        void reloadBackend();
+        bool reloadBackend();
 
     private:
         friend class DeleteCollectionPrompt;

src/fdosecrets/objects/DBusObject.cpp

@@ -31,17 +31,18 @@ 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, QDBusAbstractAdaptor* adaptor)
     {
+        Q_ASSERT(!m_dbusAdaptor);
+
         m_objectPath.setPath(path);
         m_dbusAdaptor = adaptor;
         adaptor->setParent(this);
-        auto ok = QDBusConnection::sessionBus().registerObject(m_objectPath.path(), this);
-        Q_UNUSED(ok);
-        Q_ASSERT(ok);
+        return QDBusConnection::sessionBus().registerObject(m_objectPath.path(), this);
     }
 
     QString DBusObject::callingPeerName() const

src/fdosecrets/objects/DBusObject.h

@@ -57,7 +57,7 @@ namespace FdoSecrets
         }
 
     protected:
-        void registerWithPath(const QString& path, QDBusAbstractAdaptor* adaptor);
+        bool registerWithPath(const QString& path, QDBusAbstractAdaptor* adaptor);
 
         void unregisterCurrentPath()
         {

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

@@ -48,18 +48,36 @@ namespace FdoSecrets
         constexpr auto FDO_SECRETS_CONTENT_TYPE = "FDO_SECRETS_CONTENT_TYPE";
     } // namespace
 
+    Item* Item::Create(Collection* parent, Entry* backend)
+    {
+        std::unique_ptr<Item> res{new Item(parent, backend)};
+
+        if (!res->registerSelf()) {
+            return nullptr;
+        }
+
+        return res.release();
+    }
+
     Item::Item(Collection* parent, Entry* backend)
         : DBusObject(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, new ItemAdaptor(this));
+        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);
-        return prompt;
+        auto prompt = DeleteItemPrompt::Create(service(), this);
+        return prompt.value();
     }
 
     DBusReturn<SecretStruct> Item::getSecret(Session* session)

src/fdosecrets/objects/Item.h

@@ -41,8 +41,18 @@ namespace FdoSecrets
     class Item : public DBusObject
     {
         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;
 
@@ -91,6 +101,12 @@ namespace FdoSecrets
         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.