FdoSecrets: unit tests all green

- use QTRY_COMPARE when checking signal spies, as dbus signals are received in another thread
- fixes in meta type registration and type conversion
- remove QStringLiteral in COMPARE macros, making diff output readable
- other minor fixes

src/fdosecrets/FdoSecretsPlugin.cpp

@@ -18,7 +18,6 @@
 #include "FdoSecretsPlugin.h"
 
 #include "fdosecrets/FdoSecretsSettings.h"
-#include "fdosecrets/dbus/DBusTypes.h"
 #include "fdosecrets/objects/Service.h"
 #include "fdosecrets/widgets/SettingsWidgetFdoSecrets.h"
 
@@ -36,7 +35,6 @@ FdoSecretsPlugin::FdoSecretsPlugin(DatabaseTabWidget* tabWidget)
     , m_dbus(*this, QDBusConnection::sessionBus())
 {
     g_fdoSecretsPlugin = this;
-    FdoSecrets::registerDBusTypes();
 }
 
 FdoSecretsPlugin* FdoSecretsPlugin::getPlugin()

src/fdosecrets/dbus/DBusDispatch.cpp

@@ -20,10 +20,12 @@
 #include "fdosecrets/dbus/DBusObject.h"
 #include "fdosecrets/dbus/DBusTypes.h"
 #include "fdosecrets/objects/Item.h"
+#include "fdosecrets/objects/Service.h"
 
 #include "core/Global.h"
 
 #include <QThread>
+#include <QDBusMetaType>
 
 namespace FdoSecrets
 {
@@ -38,44 +40,60 @@ namespace FdoSecrets
 
     bool prepareInputParams(const QString& signature,
                             const QVector<int>& inputTypes,
-                            const QDBusMessage& msg,
+                            const QString& reqSignature,
+                            const QVariantList& args,
                             QVarLengthArray<void*, 10>& params,
                             QVariantList& auxParams)
     {
-        // msg.signature() is verified by Qt to match msg.arguments(), but the content itself is untrusted,
-        // so they need to be validated
-        if (signature != msg.signature() || inputTypes.size() != msg.arguments().size()) {
+        // requested signature is verified by Qt to match the content of arguments,
+        // but this list of arguments itself is untrusted
+        if (signature != reqSignature || inputTypes.size() != args.size()) {
+            qDebug() << "Message signature does not match, expected" << signature << inputTypes.size() << "got" << reqSignature << args.size();
             return false;
         }
 
         // prepare params
         for (int count = 0; count != inputTypes.size(); ++count) {
             const auto& id = inputTypes.at(count);
-            const auto& arg = msg.arguments().at(count);
+            const auto& arg = args.at(count);
 
             if (arg.userType() == id) {
-                // no conversion needed
+                // shortcut for no conversion
                 params.append(const_cast<void*>(arg.constData()));
-            } else if (arg.canConvert(id)) {
-                // make a copy to store the converted value
-                auxParams.append(arg);
-                auto& out = auxParams.last();
-                if (!out.convert(id)) {
-                    qDebug() << "Internal error: failed conversion from" << arg << "to type id" << id;
+                continue;
+            }
+
+            // we need at least one conversion, allocate a slot in auxParams
+            auxParams.append(QVariant(id, nullptr));
+            auto& out = auxParams.last();
+            // first handle QDBusArgument to wire types
+            if (arg.userType() == qMetaTypeId<QDBusArgument>()) {
+                auto wireId = typeToWireType(id).dbusTypeId;
+                out = QVariant(wireId, nullptr);
+
+                const auto& in = arg.value<QDBusArgument>();
+                if (!QDBusMetaType::demarshall(in, wireId, out.data())) {
+                    qDebug() << "Internal error: failed QDBusArgument conversion from" << arg << "to type" << QMetaType::typeName(wireId) << wireId;
                     return false;
                 }
-                params.append(const_cast<void*>(out.constData()));
             } else {
-                qDebug() << "Internal error: unhandled new parameter type id" << id << "and arg" << arg;
+                // make a copy to store the converted value
+                out = arg;
+            }
+            // other conversions are handled here
+            if (!out.convert(id)) {
+                qDebug() << "Internal error: failed conversion from" << arg << "to type" << QMetaType::typeName(id) << id;
                 return false;
             }
+            // good to go
+            params.append(const_cast<void*>(out.constData()));
         }
         return true;
     }
 
     void DBusMgr::populateMethodCache(const QMetaObject& mo)
     {
-        for (int i = 0; i != mo.methodOffset(); ++i) {
+        for (int i = mo.methodOffset(); i != mo.methodCount(); ++i) {
             auto mm = mo.method(i);
 
             // only register public Q_INVOKABLE methods
@@ -140,10 +158,72 @@ namespace FdoSecrets
         }
     }
 
-    bool DBusMgr::activateObject(const QString& path,
-                                 const QString& interface,
-                                 const QString& member,
-                                 const QDBusMessage& msg)
+    bool DBusMgr::handleMessage(const QDBusMessage& message, const QDBusConnection&)
+    {
+//        qDebug() << "DBusMgr::handleMessage: " << message;
+
+        // save a mutable copy of the message, as we may modify it to unify property access
+        // and method call
+        RequestedMethod req{
+            message.interface(),
+            message.member(),
+            message.signature(),
+            message.arguments(),
+        };
+
+        // introspection is handled by returning false
+        // but we need to handle properties ourselves like regular functions
+        if (req.interface == QLatin1String("org.freedesktop.DBus.Properties")) {
+            if (req.signature != "ss") {
+                // invalid message
+                qDebug() << "Invalid message" << message;
+                return false;
+            }
+
+            req.interface = req.args.at(0).toString();
+            // Set + Name
+            if (req.member == QLatin1String("Set")) {
+                req.member = req.member + req.args.at(1).toString();
+            } else {
+                req.member = req.args.at(1).toString();
+            }
+            req.signature = "";
+            req.args = {};
+        }
+        // mapping from dbus member name to function names on the object
+        req.member = pascalToCamel(req.member);
+        // also "delete" => "remove" due to c++ keyword restriction
+        if (req.member == QLatin1Literal("delete")) {
+            req.member = QLatin1Literal("remove");
+        }
+
+        // from now on we may call into dbus objects, so setup client first
+        auto client = findClient(message.service());
+        if (!client) {
+            // the client already died
+            return false;
+        }
+
+        struct ContextSetter
+        {
+            explicit ContextSetter(DBusClientPtr client)
+            {
+                old = std::move(Context);
+                Context = std::move(client);
+            }
+            ~ContextSetter()
+            {
+                Context = std::move(old);
+            }
+            DBusClientPtr old;
+        };
+        ContextSetter s(std::move(client));
+
+        // activate the target object
+        return activateObject(message.path(), req, message);
+    }
+
+    bool DBusMgr::activateObject(const QString& path, const RequestedMethod& req, const QDBusMessage& msg)
     {
         auto obj = m_objects.value(path, nullptr);
         if (!obj) {
@@ -156,16 +236,16 @@ namespace FdoSecrets
 
         auto mo = obj->metaObject();
         // check if interface matches
-        if (interface != mo->classInfo(mo->indexOfClassInfo("D-Bus Interface")).value()) {
+        if (req.interface != mo->classInfo(mo->indexOfClassInfo("D-Bus Interface")).value()) {
             qDebug() << "DBusMgr::handleMessage with mismatch interface";
             return false;
         }
 
         // find the slot to call
-        auto cacheKey = QStringLiteral("%1.%2").arg(interface, member);
+        auto cacheKey = QStringLiteral("%1.%2").arg(req.interface, req.member);
         auto it = m_cachedMethods.find(cacheKey);
         if (it == m_cachedMethods.end()) {
-            qDebug() << "DBusMgr::handleMessage with nonexisting method";
+            qDebug() << "DBusMgr::handleMessage with nonexisting method" << cacheKey;
             return false;
         }
 
@@ -177,7 +257,8 @@ namespace FdoSecrets
         params.append(&ret);
 
         // prepare input
-        if (!prepareInputParams(it->signature, it->inputTypes, msg, params, auxParams)) {
+        if (!prepareInputParams(it->signature, it->inputTypes, req.signature, req.args, params, auxParams)) {
+            qDebug() << "Failed to prepare input params";
             return false;
         }
 
@@ -214,52 +295,4 @@ namespace FdoSecrets
         }
         return sendDBus(msg.createReply(outputArgs));
     }
-
-    bool DBusMgr::handleMessage(const QDBusMessage& message, const QDBusConnection&)
-    {
-        qDebug() << "DBusMgr::handleMessage: " << message;
-
-        auto iface = message.interface();
-        auto member = message.member();
-        // introspection is handled by returning false
-        // but we need to handle properties ourselves like regular functions
-        if (iface == QLatin1String("org.freedesktop.DBus.Properties")) {
-            iface = message.arguments().at(0).toString();
-            // Set + Name
-            if (member == QLatin1String("Set")) {
-                member = member + message.arguments().at(1).toString();
-            }
-        }
-        // mapping from dbus member name to function names on the object
-        member = pascalToCamel(member);
-        // also "delete" => "remove" due to c++ keyword
-        if (member == QLatin1Literal("delete")) {
-            member = QLatin1Literal("remove");
-        }
-
-        // from now on we may call into dbus objects, so setup client first
-        auto client = findClient(message.service());
-        if (!client) {
-            // the client already died
-            return false;
-        }
-
-        struct ContextSetter
-        {
-            explicit ContextSetter(DBusClientPtr client)
-            {
-                old = std::move(Context);
-                Context = std::move(client);
-            }
-            ~ContextSetter()
-            {
-                Context = std::move(old);
-            }
-            DBusClientPtr old;
-        };
-        ContextSetter s(std::move(client));
-
-        // activate the target object
-        return activateObject(message.path(), iface, member, message);
-    }
 } // namespace FdoSecrets

src/fdosecrets/dbus/DBusMgr.cpp

@@ -44,6 +44,8 @@ namespace FdoSecrets
         : m_plugin(plugin)
         , m_conn(std::move(conn))
     {
+        registerDBusTypes();
+
         // these are the methods we expose on DBus
         populateMethodCache(Service::staticMetaObject);
         populateMethodCache(Collection::staticMetaObject);
@@ -68,7 +70,6 @@ namespace FdoSecrets
 
     void DBusMgr::overrideClient(const DBusClientPtr& fake) const
     {
-        Q_ASSERT(!Context);
         Context = fake;
     }
 
@@ -210,14 +211,17 @@ namespace FdoSecrets
             .arg(otherService);
     }
 
-    bool DBusMgr::registerObject(const QString& path, DBusObject* obj)
+    bool DBusMgr::registerObject(const QString& path, DBusObject* obj, bool primary)
     {
         if (!m_conn.registerVirtualObject(path, this)) {
+            qDebug() << "failed to register" << obj << "at" << path;
             return false;
         }
         connect(obj, &DBusObject::destroyed, this, &DBusMgr::unregisterObject);
         m_objects.insert(path, obj);
-        obj->setObjectPath(path);
+        if (primary) {
+            obj->setObjectPath(path);
+        }
         return true;
     }
 
@@ -254,11 +258,12 @@ namespace FdoSecrets
         auto name = encodePath(coll->name());
         auto path = QLatin1Literal(DBUS_PATH_TEMPLATE_COLLECTION).arg(QLatin1Literal(DBUS_PATH_SECRETS), name);
         if (!registerObject(path, coll)) {
+
             // try again with a suffix
             name += QLatin1Literal("_%1").arg(Tools::uuidToHex(QUuid::createUuid()).left(4));
             path = QLatin1Literal(DBUS_PATH_TEMPLATE_COLLECTION).arg(QLatin1Literal(DBUS_PATH_SECRETS), name);
 
-            if (!m_conn.registerVirtualObject(path, this)) {
+            if (!registerObject(path, coll)) {
                 qDebug() << "Failed to register database on DBus under name" << name;
                 m_plugin.emitError(tr("Failed to register database on DBus under the name '%1'").arg(name));
                 return false;
@@ -319,12 +324,16 @@ namespace FdoSecrets
     bool DBusMgr::registerAlias(Collection* coll, const QString& alias)
     {
         auto path = QLatin1Literal(DBUS_PATH_TEMPLATE_ALIAS).arg(QLatin1Literal(DBUS_PATH_SECRETS), alias);
-        if (!registerObject(path, coll)) {
+        if (!registerObject(path, coll, false)) {
             qDebug() << "Failed to register database on DBus under alias" << alias;
             // usually this is reported back directly on dbus, so no need to show in UI
             return false;
         }
         // alias signals are handled together with collections' primary path in emitCollection*
+        // but we need to handle object destroy here
+        connect(coll, &DBusObject::destroyed, this, [this, alias]() {
+            unregisterAlias(alias);
+        });
         return true;
     }
 

src/fdosecrets/dbus/DBusMgr.h

@@ -20,12 +20,15 @@
 
 #include "fdosecrets/dbus/DBusClient.h"
 
+#include <QByteArray>
 #include <QDBusConnection>
 #include <QDBusObjectPath>
 #include <QDBusServiceWatcher>
 #include <QDBusVirtualObject>
+#include <QDebug>
 #include <QHash>
 #include <QPointer>
+#include <QVector>
 
 #include <utility>
 
@@ -153,12 +156,18 @@ namespace FdoSecrets
         template <typename T> static T* pathToObject(const QDBusObjectPath& path)
         {
             if (!Context) {
+                qDebug() << "No context when looking up path" << path.path();
                 return nullptr;
             }
             if (path.path() == QStringLiteral("/")) {
                 return nullptr;
             }
-            return qobject_cast<T*>(Context->dbus().m_objects.value(path.path(), nullptr));
+            auto obj = qobject_cast<T*>(Context->dbus().m_objects.value(path.path(), nullptr));
+            if (!obj) {
+                qDebug() << "object not found at path" << path.path();
+                qDebug() << Context->dbus().m_objects;
+            }
+            return obj;
         }
 
         /**
@@ -249,7 +258,7 @@ namespace FdoSecrets
             }
         };
         ParsedPath parsePath(const QString& path) const;
-        bool registerObject(const QString& path, DBusObject* obj);
+        bool registerObject(const QString& path, DBusObject* obj, bool primary = true);
 
         // method dispatching
         struct MethodData
@@ -262,8 +271,15 @@ namespace FdoSecrets
         };
         QHash<QString, MethodData> m_cachedMethods{};
         void populateMethodCache(const QMetaObject& mo);
-        bool
-        activateObject(const QString& path, const QString& interface, const QString& member, const QDBusMessage& msg);
+
+        struct RequestedMethod
+        {
+            QString interface;
+            QString member;
+            QString signature;
+            QVariantList args;
+        };
+        bool activateObject(const QString& path, const RequestedMethod& req, const QDBusMessage& msg);
 
         // client management
         friend class DBusClient;