From 000e1823acbf84f36919c8362e7994755c9b4e13 Mon Sep 17 00:00:00 2001 From: Aetf Date: Mon, 2 Nov 2020 22:58:54 -0500 Subject: [PATCH 1/6] FdoSecrets: refactor DBus registration error handling --- src/fdosecrets/FdoSecretsPlugin.cpp | 17 ++++---- src/fdosecrets/FdoSecretsPlugin.h | 11 ++++- src/fdosecrets/objects/Collection.cpp | 37 ++++++++++++---- src/fdosecrets/objects/Collection.h | 17 ++++++-- src/fdosecrets/objects/DBusObject.cpp | 9 ++-- src/fdosecrets/objects/DBusObject.h | 2 +- src/fdosecrets/objects/DBusTypes.h | 1 + src/fdosecrets/objects/Item.cpp | 28 +++++++++--- src/fdosecrets/objects/Item.h | 18 +++++++- src/fdosecrets/objects/Prompt.cpp | 60 ++++++++++++++++++++++++-- src/fdosecrets/objects/Prompt.h | 22 +++++++--- src/fdosecrets/objects/Service.cpp | 61 +++++++++++++++++++-------- src/fdosecrets/objects/Service.h | 27 ++++++------ src/fdosecrets/objects/Session.cpp | 29 ++++++++++++- src/fdosecrets/objects/Session.h | 18 +++++++- 15 files changed, 281 insertions(+), 76 deletions(-) diff --git a/src/fdosecrets/FdoSecretsPlugin.cpp b/src/fdosecrets/FdoSecretsPlugin.cpp index 96d76e34..e75a77ae 100644 --- a/src/fdosecrets/FdoSecretsPlugin.cpp +++ b/src/fdosecrets/FdoSecretsPlugin.cpp @@ -26,8 +26,6 @@ #include -#include - 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("Fdo Secret Service: %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("Fdo Secret Service: %1").arg(msg)); +} + QString FdoSecretsPlugin::reportExistingService() const { auto pidStr = tr("Unknown", "Unknown PID"); diff --git a/src/fdosecrets/FdoSecretsPlugin.h b/src/fdosecrets/FdoSecretsPlugin.h index ec57fec8..fdf2aba5 100644 --- a/src/fdosecrets/FdoSecretsPlugin.h +++ b/src/fdosecrets/FdoSecretsPlugin.h @@ -22,7 +22,8 @@ #include "gui/Icons.h" #include -#include + +#include 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 m_dbTabs; - QScopedPointer m_secretService; + std::unique_ptr m_secretService; }; #endif // KEEPASSXC_FDOSECRETSPLUGIN_H diff --git a/src/fdosecrets/objects/Collection.cpp b/src/fdosecrets/objects/Collection.cpp index c0bb8ff3..f03b9209 100644 --- a/src/fdosecrets/objects/Collection.cpp +++ b/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 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 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; diff --git a/src/fdosecrets/objects/Collection.h b/src/fdosecrets/objects/Collection.h index da6dcb38..5f06cc33 100644 --- a/src/fdosecrets/objects/Collection.h +++ b/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> 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; diff --git a/src/fdosecrets/objects/DBusObject.cpp b/src/fdosecrets/objects/DBusObject.cpp index 8bf1ae4e..c53baa72 100644 --- a/src/fdosecrets/objects/DBusObject.cpp +++ b/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 diff --git a/src/fdosecrets/objects/DBusObject.h b/src/fdosecrets/objects/DBusObject.h index 4cdaf5ce..eeb50507 100644 --- a/src/fdosecrets/objects/DBusObject.h +++ b/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() { diff --git a/src/fdosecrets/objects/DBusTypes.h b/src/fdosecrets/objects/DBusTypes.h index 384a1e6b..ef1e2276 100644 --- a/src/fdosecrets/objects/DBusTypes.h +++ b/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 { diff --git a/src/fdosecrets/objects/Item.cpp b/src/fdosecrets/objects/Item.cpp index d58cbcc2..038b9a92 100644 --- a/src/fdosecrets/objects/Item.cpp +++ b/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 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 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 Item::getSecret(Session* session) diff --git a/src/fdosecrets/objects/Item.h b/src/fdosecrets/objects/Item.h index 99601d95..746850e7 100644 --- a/src/fdosecrets/objects/Item.h +++ b/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 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. diff --git a/src/fdosecrets/objects/Prompt.cpp b/src/fdosecrets/objects/Prompt.cpp index cb3cb4f4..b20be86f 100644 --- a/src/fdosecrets/objects/Prompt.cpp +++ b/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" @@ -35,13 +36,19 @@ namespace FdoSecrets PromptBase::PromptBase(Service* parent) : DBusObject(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, new PromptAdaptor(this)); + 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 +76,15 @@ namespace FdoSecrets return {}; } + DBusReturn DeleteCollectionPrompt::Create(Service* parent, Collection* coll) + { + std::unique_ptr res{new DeleteCollectionPrompt(parent, coll)}; + if (!res->registerSelf()) { + return DBusReturn<>::Error(QDBusError::InvalidObjectPath); + } + return res.release(); + } + DeleteCollectionPrompt::DeleteCollectionPrompt(Service* parent, Collection* coll) : PromptBase(parent) , m_collection(coll) @@ -100,6 +116,15 @@ namespace FdoSecrets return {}; } + DBusReturn CreateCollectionPrompt::Create(Service* parent) + { + std::unique_ptr res{new CreateCollectionPrompt(parent)}; + if (!res->registerSelf()) { + return DBusReturn<>::Error(QDBusError::InvalidObjectPath); + } + return res.release(); + } + CreateCollectionPrompt::CreateCollectionPrompt(Service* parent) : PromptBase(parent) { @@ -136,6 +161,15 @@ namespace FdoSecrets return {}; } + DBusReturn LockCollectionsPrompt::Create(Service* parent, const QList& colls) + { + std::unique_ptr res{new LockCollectionsPrompt(parent, colls)}; + if (!res->registerSelf()) { + return DBusReturn<>::Error(QDBusError::InvalidObjectPath); + } + return res.release(); + } + LockCollectionsPrompt::LockCollectionsPrompt(Service* parent, const QList& colls) : PromptBase(parent) { @@ -179,6 +213,15 @@ namespace FdoSecrets return {}; } + DBusReturn UnlockCollectionsPrompt::Create(Service* parent, const QList& coll) + { + std::unique_ptr res{new UnlockCollectionsPrompt(parent, coll)}; + if (!res->registerSelf()) { + return DBusReturn<>::Error(QDBusError::InvalidObjectPath); + } + return res.release(); + } + UnlockCollectionsPrompt::UnlockCollectionsPrompt(Service* parent, const QList& colls) : PromptBase(parent) { @@ -246,6 +289,15 @@ namespace FdoSecrets return {}; } + DBusReturn DeleteItemPrompt::Create(Service* parent, Item* item) + { + std::unique_ptr res{new DeleteItemPrompt(parent, item)}; + if (!res->registerSelf()) { + return DBusReturn<>::Error(QDBusError::InvalidObjectPath); + } + return res.release(); + } + DeleteItemPrompt::DeleteItemPrompt(Service* parent, Item* item) : PromptBase(parent) , m_item(item) diff --git a/src/fdosecrets/objects/Prompt.h b/src/fdosecrets/objects/Prompt.h index 683642df..10939dcb 100644 --- a/src/fdosecrets/objects/Prompt.h +++ b/src/fdosecrets/objects/Prompt.h @@ -36,8 +36,6 @@ namespace FdoSecrets { Q_OBJECT public: - explicit PromptBase(Service* parent); - virtual DBusReturn prompt(const QString& windowId) = 0; virtual DBusReturn 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,8 +57,9 @@ namespace FdoSecrets { Q_OBJECT - public: explicit DeleteCollectionPrompt(Service* parent, Collection* coll); + public: + static DBusReturn Create(Service* parent, Collection* coll); DBusReturn prompt(const QString& windowId) override; @@ -69,8 +71,9 @@ namespace FdoSecrets { Q_OBJECT - public: explicit CreateCollectionPrompt(Service* parent); + public: + static DBusReturn Create(Service* parent); DBusReturn prompt(const QString& windowId) override; DBusReturn dismiss() override; @@ -82,8 +85,10 @@ namespace FdoSecrets class LockCollectionsPrompt : public PromptBase { Q_OBJECT - public: + explicit LockCollectionsPrompt(Service* parent, const QList& colls); + public: + static DBusReturn Create(Service* parent, const QList& colls); DBusReturn prompt(const QString& windowId) override; DBusReturn dismiss() override; @@ -96,8 +101,10 @@ namespace FdoSecrets class UnlockCollectionsPrompt : public PromptBase { Q_OBJECT - public: + explicit UnlockCollectionsPrompt(Service* parent, const QList& coll); + public: + static DBusReturn Create(Service* parent, const QList& coll); DBusReturn prompt(const QString& windowId) override; DBusReturn dismiss() override; @@ -116,8 +123,9 @@ namespace FdoSecrets { Q_OBJECT - public: explicit DeleteItemPrompt(Service* parent, Item* item); + public: + static DBusReturn Create(Service* parent, Item* item); DBusReturn prompt(const QString& windowId) override; diff --git a/src/fdosecrets/objects/Service.cpp b/src/fdosecrets/objects/Service.cpp index 892ba68a..34ace721 100644 --- a/src/fdosecrets/objects/Service.cpp +++ b/src/fdosecrets/objects/Service.cpp @@ -31,6 +31,8 @@ #include #include +#include + namespace { constexpr auto DEFAULT_ALIAS = "default"; @@ -38,6 +40,14 @@ namespace namespace FdoSecrets { + std::unique_ptr Service::Create(FdoSecretsPlugin* plugin, QPointer dbTabs) + { + std::unique_ptr res{new Service(plugin, std::move(dbTabs))}; + if (!res->initialize()) { + return {}; + } + return res; + } Service::Service(FdoSecretsPlugin* plugin, QPointer dbTabs) // clazy: exclude=ctor-missing-parent-argument @@ -59,16 +69,19 @@ namespace FdoSecrets bool Service::initialize() { if (!QDBusConnection::sessionBus().registerService(QStringLiteral(DBUS_SERVICE_SECRET))) { - emit error(tr("Failed to register DBus service at %1.
").arg(QLatin1String(DBUS_SERVICE_SECRET)) + plugin()->emitError(tr("Failed to register DBus service at %1.
").arg(QLatin1String(DBUS_SERVICE_SECRET)) + m_plugin->reportExistingService()); return false; } - registerWithPath(QStringLiteral(DBUS_PATH_SECRETS), new ServiceAdaptor(this)); + if (!registerWithPath(QStringLiteral(DBUS_PATH_SECRETS), new ServiceAdaptor(this))) { + plugin()->emitError(tr("Failed to register DBus path %1.
").arg(QStringLiteral(DBUS_PATH_SECRETS))); + return false; + } // Connect to service unregistered signal m_serviceWatcher.reset(new QDBusServiceWatcher()); - connect(m_serviceWatcher.data(), + connect(m_serviceWatcher.get(), &QDBusServiceWatcher::serviceUnregistered, this, &Service::dbusServiceUnregistered); @@ -106,11 +119,10 @@ namespace FdoSecrets monitorDatabaseExposedGroup(dbWidget); }); - auto coll = new Collection(this, dbWidget); - // Creation may fail if the database is not exposed. - // This is okay, because we monitor the expose settings above - if (!coll->isValid()) { - coll->deleteLater(); + auto coll = Collection::Create(this, dbWidget); + if (!coll) { + // The creation may fail if the database is not exposed. + // This is okay, because we monitor the expose settings above return; } @@ -184,8 +196,9 @@ namespace FdoSecrets Q_ASSERT(m_serviceWatcher); auto removed = m_serviceWatcher->removeWatchedService(service); - Q_UNUSED(removed); - Q_ASSERT(removed); + if (!removed) { + qDebug("FdoSecrets: Failed to remove service watcher"); + } Session::CleanupNegotiation(service); auto sess = m_peerToSession.value(service, nullptr); @@ -218,7 +231,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,12 +256,15 @@ namespace FdoSecrets // return existing collection if alias is non-empty and exists. auto collection = findCollection(alias); if (!collection) { - auto cp = new CreateCollectionPrompt(this); - prompt = cp; + auto cp = CreateCollectionPrompt::Create(this); + 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(), &CreateCollectionPrompt::collectionCreated, cp.value(), [alias, properties](Collection* coll) { coll->setProperties(properties).okOrDie(); if (!alias.isEmpty()) { coll->addAlias(alias).okOrDie(); @@ -314,7 +333,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 +375,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; } diff --git a/src/fdosecrets/objects/Service.h b/src/fdosecrets/objects/Service.h index 4e0eeb0f..d8d7529d 100644 --- a/src/fdosecrets/objects/Service.h +++ b/src/fdosecrets/objects/Service.h @@ -24,9 +24,10 @@ #include #include #include -#include #include +#include + class QDBusServiceWatcher; class DatabaseTabWidget; @@ -47,11 +48,17 @@ namespace FdoSecrets class Service : public DBusObject // clazy: exclude=ctor-missing-parent-argument { Q_OBJECT - public: - explicit Service(FdoSecretsPlugin* plugin, QPointer dbTabs); - ~Service() override; - bool initialize(); + explicit Service(FdoSecretsPlugin* plugin, QPointer 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 std::unique_ptr Create(FdoSecretsPlugin* plugin, QPointer dbTabs); + ~Service() override; DBusReturn openSession(const QString& algorithm, const QVariant& input, Session*& result); DBusReturn @@ -82,12 +89,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 +132,8 @@ namespace FdoSecrets void onCollectionAliasRemoved(const QString& alias); private: + bool initialize(); + /** * Find collection by alias name * @param alias @@ -158,7 +161,7 @@ namespace FdoSecrets bool m_insdieEnsureDefaultAlias; - QScopedPointer m_serviceWatcher; + std::unique_ptr m_serviceWatcher; }; } // namespace FdoSecrets diff --git a/src/fdosecrets/objects/Session.cpp b/src/fdosecrets/objects/Session.cpp index 4845752a..1d502019 100644 --- a/src/fdosecrets/objects/Session.cpp +++ b/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" @@ -25,14 +26,33 @@ namespace FdoSecrets QHash Session::negoniationState; + Session* Session::Create(std::unique_ptr&& cipher, const QString& peer, Service* parent) + { + std::unique_ptr res{new Session(std::move(cipher), peer, parent)}; + + if (!res->registerSelf()) { + return nullptr; + } + + return res.release(); + } + Session::Session(std::unique_ptr&& cipher, const QString& peer, Service* parent) : DBusObject(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, new SessionAdaptor(this)); + 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) @@ -58,6 +78,11 @@ namespace FdoSecrets return Tools::uuidToHex(m_id); } + Service* Session::service() const + { + return qobject_cast(parent()); + } + std::unique_ptr Session::CreateCiphers(const QString& peer, const QString& algorithm, const QVariant& input, diff --git a/src/fdosecrets/objects/Session.h b/src/fdosecrets/objects/Session.h index 472cc6e5..199a7ccc 100644 --- a/src/fdosecrets/objects/Session.h +++ b/src/fdosecrets/objects/Session.h @@ -37,6 +37,8 @@ namespace FdoSecrets class Session : public DBusObject { Q_OBJECT + + explicit Session(std::unique_ptr&& cipher, const QString& peer, Service* parent); public: static std::unique_ptr CreateCiphers(const QString& peer, const QString& algorithm, @@ -45,7 +47,16 @@ namespace FdoSecrets bool& incomplete); static void CleanupNegotiation(const QString& peer); - explicit Session(std::unique_ptr&& 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&& cipher, const QString& peer, Service* parent); DBusReturn close(); @@ -71,6 +82,8 @@ namespace FdoSecrets QString id() const; + Service* service() const; + signals: /** * The session is going to be closed @@ -78,6 +91,9 @@ namespace FdoSecrets */ void aboutToClose(); + private: + bool registerSelf(); + private: std::unique_ptr m_cipher; QString m_peer; From f5caf3968f57e85eaddf67407190db6d338b4365 Mon Sep 17 00:00:00 2001 From: Aetf Date: Mon, 2 Nov 2020 23:01:04 -0500 Subject: [PATCH 2/6] FdoSecrets: fix typos --- src/fdosecrets/objects/Collection.cpp | 11 +++++------ src/fdosecrets/objects/Service.cpp | 8 ++++---- src/fdosecrets/objects/Service.h | 2 +- src/fdosecrets/objects/Session.cpp | 4 ++-- src/fdosecrets/objects/Session.h | 4 ++-- tests/gui/TestGuiFdoSecrets.cpp | 4 ++-- tests/gui/TestGuiFdoSecrets.h | 2 +- 7 files changed, 17 insertions(+), 18 deletions(-) diff --git a/src/fdosecrets/objects/Collection.cpp b/src/fdosecrets/objects/Collection.cpp index f03b9209..90d6d9e8 100644 --- a/src/fdosecrets/objects/Collection.cpp +++ b/src/fdosecrets/objects/Collection.cpp @@ -25,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" @@ -330,12 +329,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; } } diff --git a/src/fdosecrets/objects/Service.cpp b/src/fdosecrets/objects/Service.cpp index 34ace721..2577b476 100644 --- a/src/fdosecrets/objects/Service.cpp +++ b/src/fdosecrets/objects/Service.cpp @@ -54,7 +54,7 @@ namespace FdoSecrets : DBusObject(nullptr) , m_plugin(plugin) , m_databases(std::move(dbTabs)) - , m_insdieEnsureDefaultAlias(false) + , m_insideEnsureDefaultAlias(false) , m_serviceWatcher(nullptr) { connect( @@ -176,11 +176,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) { @@ -188,7 +188,7 @@ namespace FdoSecrets coll->addAlias(DEFAULT_ALIAS).okOrDie(); } - m_insdieEnsureDefaultAlias = false; + m_insideEnsureDefaultAlias = false; } void Service::dbusServiceUnregistered(const QString& service) diff --git a/src/fdosecrets/objects/Service.h b/src/fdosecrets/objects/Service.h index d8d7529d..cf27b277 100644 --- a/src/fdosecrets/objects/Service.h +++ b/src/fdosecrets/objects/Service.h @@ -159,7 +159,7 @@ namespace FdoSecrets QList m_sessions; QHash m_peerToSession; - bool m_insdieEnsureDefaultAlias; + bool m_insideEnsureDefaultAlias; std::unique_ptr m_serviceWatcher; }; diff --git a/src/fdosecrets/objects/Session.cpp b/src/fdosecrets/objects/Session.cpp index 1d502019..26a3c0cf 100644 --- a/src/fdosecrets/objects/Session.cpp +++ b/src/fdosecrets/objects/Session.cpp @@ -24,7 +24,7 @@ namespace FdoSecrets { - QHash Session::negoniationState; + QHash Session::negotiationState; Session* Session::Create(std::unique_ptr&& cipher, const QString& peer, Service* parent) { @@ -57,7 +57,7 @@ namespace FdoSecrets void Session::CleanupNegotiation(const QString& peer) { - negoniationState.remove(peer); + negotiationState.remove(peer); } DBusReturn Session::close() diff --git a/src/fdosecrets/objects/Session.h b/src/fdosecrets/objects/Session.h index 199a7ccc..58b971fc 100644 --- a/src/fdosecrets/objects/Session.h +++ b/src/fdosecrets/objects/Session.h @@ -42,7 +42,7 @@ namespace FdoSecrets public: static std::unique_ptr CreateCiphers(const QString& peer, const QString& algorithm, - const QVariant& intpu, + const QVariant& input, QVariant& output, bool& incomplete); static void CleanupNegotiation(const QString& peer); @@ -99,7 +99,7 @@ namespace FdoSecrets QString m_peer; QUuid m_id; - static QHash negoniationState; + static QHash negotiationState; }; } // namespace FdoSecrets diff --git a/tests/gui/TestGuiFdoSecrets.cpp b/tests/gui/TestGuiFdoSecrets.cpp index e19e6130..af8f32f3 100644 --- a/tests/gui/TestGuiFdoSecrets.cpp +++ b/tests/gui/TestGuiFdoSecrets.cpp @@ -267,7 +267,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()); } @@ -1047,7 +1047,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"); diff --git a/tests/gui/TestGuiFdoSecrets.h b/tests/gui/TestGuiFdoSecrets.h index 8a961eb8..c4ed108e 100644 --- a/tests/gui/TestGuiFdoSecrets.h +++ b/tests/gui/TestGuiFdoSecrets.h @@ -82,7 +82,7 @@ private slots: void testDefaultAliasAlwaysPresent(); void testExposeSubgroup(); - void testModifiyingExposedGroup(); + void testModifyingExposedGroup(); protected slots: void createDatabaseCallback(); From 804a3b6706c96d95aa66a9d9f291b3a7cf181118 Mon Sep 17 00:00:00 2001 From: Aetf Date: Tue, 3 Nov 2020 13:06:04 -0500 Subject: [PATCH 3/6] FdoSecrets: simplify collection internal states This gets rid of the m_registered state, so whenever there is a valid m_backend, it is guaranteed to be registered already. While at it, this commit also improves DBusObject::registerWithPath a little bit by allowing properly registering multiple paths using the same adaptor, mostly for supporting Collection aliases. Now when DBus registration fails, the code does not go into an inconsistent state or crash. --- src/fdosecrets/FdoSecretsPlugin.cpp | 1 + src/fdosecrets/objects/Collection.cpp | 59 +++++++++++++-------------- src/fdosecrets/objects/Collection.h | 8 ++-- src/fdosecrets/objects/DBusObject.cpp | 19 ++++----- src/fdosecrets/objects/DBusObject.h | 42 +++++++++++++++---- src/fdosecrets/objects/Item.cpp | 6 +-- src/fdosecrets/objects/Item.h | 3 +- src/fdosecrets/objects/Prompt.cpp | 4 +- src/fdosecrets/objects/Prompt.h | 2 +- src/fdosecrets/objects/Service.cpp | 12 +++--- src/fdosecrets/objects/Service.h | 2 +- src/fdosecrets/objects/Session.cpp | 4 +- src/fdosecrets/objects/Session.h | 2 +- 13 files changed, 94 insertions(+), 70 deletions(-) diff --git a/src/fdosecrets/FdoSecretsPlugin.cpp b/src/fdosecrets/FdoSecretsPlugin.cpp index e75a77ae..6d44c98c 100644 --- a/src/fdosecrets/FdoSecretsPlugin.cpp +++ b/src/fdosecrets/FdoSecretsPlugin.cpp @@ -104,6 +104,7 @@ void FdoSecretsPlugin::emitRequestShowNotification(const QString& msg, const QSt void FdoSecretsPlugin::emitError(const QString& msg) { emit error(tr("Fdo Secret Service: %1").arg(msg)); + qDebug() << msg; } QString FdoSecretsPlugin::reportExistingService() const diff --git a/src/fdosecrets/objects/Collection.cpp b/src/fdosecrets/objects/Collection.cpp index 90d6d9e8..a5443fde 100644 --- a/src/fdosecrets/objects/Collection.cpp +++ b/src/fdosecrets/objects/Collection.cpp @@ -48,14 +48,13 @@ namespace FdoSecrets } 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); @@ -72,33 +71,24 @@ namespace FdoSecrets 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); + // 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(); + 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()) { - 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; + auto path = QStringLiteral(DBUS_PATH_TEMPLATE_COLLECTION).arg(p()->objectPath().path(), encodePath(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 @@ -111,6 +101,13 @@ namespace FdoSecrets return true; } + void Collection::reloadBackendOrDelete() + { + if (!reloadBackend()) { + doDelete(); + } + } + DBusReturn Collection::ensureBackend() const { if (!m_backend) { @@ -418,8 +415,7 @@ 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); @@ -454,6 +450,7 @@ namespace FdoSecrets QString Collection::name() const { + // todo: make sure the name is never empty if (m_backendPath.isEmpty()) { // This is a newly created db without saving to file. // This name is also used to register dbus path. @@ -486,7 +483,7 @@ namespace FdoSecrets void Collection::populateContents() { - if (!m_registered) { + if (!m_backend) { return; } @@ -645,7 +642,7 @@ namespace FdoSecrets emit collectionAboutToDelete(); - unregisterCurrentPath(); + unregisterPrimaryPath(); // remove alias manually to trigger signal for (const auto& a : aliases()) { @@ -663,6 +660,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() diff --git a/src/fdosecrets/objects/Collection.h b/src/fdosecrets/objects/Collection.h index 5f06cc33..0544fc6f 100644 --- a/src/fdosecrets/objects/Collection.h +++ b/src/fdosecrets/objects/Collection.h @@ -36,7 +36,7 @@ namespace FdoSecrets class Item; class PromptBase; class Service; - class Collection : public DBusObject + class Collection : public DBusObjectHelper { Q_OBJECT @@ -124,9 +124,13 @@ namespace FdoSecrets private slots: void onDatabaseLockChanged(); void onDatabaseExposedGroupChanged(); + // force reload info from backend, potentially delete self bool reloadBackend(); + // calls reloadBackend, delete self when error + void reloadBackendOrDelete(); + private: friend class DeleteCollectionPrompt; friend class CreateCollectionPrompt; @@ -165,8 +169,6 @@ namespace FdoSecrets QSet m_aliases; QList m_items; QMap m_entryToItem; - - bool m_registered; }; } // namespace FdoSecrets diff --git a/src/fdosecrets/objects/DBusObject.cpp b/src/fdosecrets/objects/DBusObject.cpp index c53baa72..fb7533c1 100644 --- a/src/fdosecrets/objects/DBusObject.cpp +++ b/src/fdosecrets/objects/DBusObject.cpp @@ -17,15 +17,12 @@ #include "DBusObject.h" -#include #include #include #include #include #include -#include - namespace FdoSecrets { @@ -35,14 +32,13 @@ namespace FdoSecrets { } - bool DBusObject::registerWithPath(const QString& path, QDBusAbstractAdaptor* adaptor) + bool DBusObject::registerWithPath(const QString& path, bool primary) { - Q_ASSERT(!m_dbusAdaptor); + if (primary) { + m_objectPath.setPath(path); + } - m_objectPath.setPath(path); - m_dbusAdaptor = adaptor; - adaptor->setParent(this); - return QDBusConnection::sessionBus().registerObject(m_objectPath.path(), this); + return QDBusConnection::sessionBus().registerObject(path, this); } QString DBusObject::callingPeerName() const @@ -58,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('%', '_'); } diff --git a/src/fdosecrets/objects/DBusObject.h b/src/fdosecrets/objects/DBusObject.h index eeb50507..d51642a8 100644 --- a/src/fdosecrets/objects/DBusObject.h +++ b/src/fdosecrets/objects/DBusObject.h @@ -21,6 +21,7 @@ #include "fdosecrets/objects/DBusReturn.h" #include "fdosecrets/objects/DBusTypes.h" +#include #include #include #include @@ -31,21 +32,19 @@ #include #include -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: - bool 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 friend class DBusReturn; + template friend class DBusObjectHelper; - private: QDBusAbstractAdaptor* m_dbusAdaptor; QDBusObjectPath m_objectPath; }; + template 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(this)); + m_dbusAdaptor->setParent(this); + } + }; + /** * Return the object path of the pointed DBusObject, or "/" if the pointer is null * @tparam T diff --git a/src/fdosecrets/objects/Item.cpp b/src/fdosecrets/objects/Item.cpp index 038b9a92..583b3415 100644 --- a/src/fdosecrets/objects/Item.cpp +++ b/src/fdosecrets/objects/Item.cpp @@ -60,7 +60,7 @@ namespace FdoSecrets } Item::Item(Collection* parent, Entry* backend) - : DBusObject(parent) + : DBusObjectHelper(parent) , m_backend(backend) { Q_ASSERT(!p()->objectPath().path().isEmpty()); @@ -71,7 +71,7 @@ namespace FdoSecrets bool Item::registerSelf() { auto path = QStringLiteral(DBUS_PATH_TEMPLATE_ITEM).arg(p()->objectPath().path(), m_backend->uuidToHex()); - bool ok = registerWithPath(path, new ItemAdaptor(this)); + bool ok = registerWithPath(path); if (!ok) { service()->plugin()->emitError(tr("Failed to register item on DBus at path '%1'").arg(path)); } @@ -340,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(); diff --git a/src/fdosecrets/objects/Item.h b/src/fdosecrets/objects/Item.h index 746850e7..97077970 100644 --- a/src/fdosecrets/objects/Item.h +++ b/src/fdosecrets/objects/Item.h @@ -38,7 +38,7 @@ namespace FdoSecrets class Collection; class PromptBase; - class Item : public DBusObject + class Item : public DBusObjectHelper { Q_OBJECT @@ -100,7 +100,6 @@ namespace FdoSecrets public slots: void doDelete(); - private: /** * @brief Register self on DBus * @return diff --git a/src/fdosecrets/objects/Prompt.cpp b/src/fdosecrets/objects/Prompt.cpp index b20be86f..6bbc6284 100644 --- a/src/fdosecrets/objects/Prompt.cpp +++ b/src/fdosecrets/objects/Prompt.cpp @@ -34,7 +34,7 @@ namespace FdoSecrets { PromptBase::PromptBase(Service* parent) - : DBusObject(parent) + : DBusObjectHelper(parent) { connect(this, &PromptBase::completed, this, &PromptBase::deleteLater); } @@ -42,7 +42,7 @@ namespace FdoSecrets bool PromptBase::registerSelf() { auto path = QStringLiteral(DBUS_PATH_TEMPLATE_PROMPT).arg(p()->objectPath().path(), Tools::uuidToHex(QUuid::createUuid())); - bool ok = registerWithPath(path, new PromptAdaptor(this)); + bool ok = registerWithPath(path); if (!ok) { service()->plugin()->emitError(tr("Failed to register item on DBus at path '%1'").arg(path)); } diff --git a/src/fdosecrets/objects/Prompt.h b/src/fdosecrets/objects/Prompt.h index 10939dcb..e72f0512 100644 --- a/src/fdosecrets/objects/Prompt.h +++ b/src/fdosecrets/objects/Prompt.h @@ -32,7 +32,7 @@ namespace FdoSecrets class Service; - class PromptBase : public DBusObject + class PromptBase : public DBusObjectHelper { Q_OBJECT public: diff --git a/src/fdosecrets/objects/Service.cpp b/src/fdosecrets/objects/Service.cpp index 2577b476..a3b4eb19 100644 --- a/src/fdosecrets/objects/Service.cpp +++ b/src/fdosecrets/objects/Service.cpp @@ -51,7 +51,7 @@ namespace FdoSecrets Service::Service(FdoSecretsPlugin* plugin, QPointer dbTabs) // clazy: exclude=ctor-missing-parent-argument - : DBusObject(nullptr) + : DBusObjectHelper(nullptr) , m_plugin(plugin) , m_databases(std::move(dbTabs)) , m_insideEnsureDefaultAlias(false) @@ -74,7 +74,7 @@ namespace FdoSecrets return false; } - if (!registerWithPath(QStringLiteral(DBUS_PATH_SECRETS), new ServiceAdaptor(this))) { + if (!registerWithPath(QStringLiteral(DBUS_PATH_SECRETS))) { plugin()->emitError(tr("Failed to register DBus path %1.
").arg(QStringLiteral(DBUS_PATH_SECRETS))); return false; } @@ -112,12 +112,12 @@ 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 = Collection::Create(this, dbWidget); if (!coll) { @@ -129,7 +129,7 @@ namespace FdoSecrets m_collections << coll; m_dbToCollection[dbWidget] = coll; - // handle alias + // 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); diff --git a/src/fdosecrets/objects/Service.h b/src/fdosecrets/objects/Service.h index cf27b277..680df711 100644 --- a/src/fdosecrets/objects/Service.h +++ b/src/fdosecrets/objects/Service.h @@ -45,7 +45,7 @@ namespace FdoSecrets class ServiceAdaptor; class Session; - class Service : public DBusObject // clazy: exclude=ctor-missing-parent-argument + class Service : public DBusObjectHelper // clazy: exclude=ctor-missing-parent-argument { Q_OBJECT diff --git a/src/fdosecrets/objects/Session.cpp b/src/fdosecrets/objects/Session.cpp index 26a3c0cf..8a4d6939 100644 --- a/src/fdosecrets/objects/Session.cpp +++ b/src/fdosecrets/objects/Session.cpp @@ -38,7 +38,7 @@ namespace FdoSecrets } Session::Session(std::unique_ptr&& cipher, const QString& peer, Service* parent) - : DBusObject(parent) + : DBusObjectHelper(parent) , m_cipher(std::move(cipher)) , m_peer(peer) , m_id(QUuid::createUuid()) @@ -48,7 +48,7 @@ namespace FdoSecrets bool Session::registerSelf() { auto path = QStringLiteral(DBUS_PATH_TEMPLATE_SESSION).arg(p()->objectPath().path(), id()); - bool ok = registerWithPath(path, new SessionAdaptor(this)); + bool ok = registerWithPath(path); if (!ok) { service()->plugin()->emitError(tr("Failed to register session on DBus at path '%1'").arg(path)); } diff --git a/src/fdosecrets/objects/Session.h b/src/fdosecrets/objects/Session.h index 58b971fc..5722895a 100644 --- a/src/fdosecrets/objects/Session.h +++ b/src/fdosecrets/objects/Session.h @@ -34,7 +34,7 @@ namespace FdoSecrets { class CipherPair; - class Session : public DBusObject + class Session : public DBusObjectHelper { Q_OBJECT From a651d7049dcd69cab6ecaecda969767fb827b303 Mon Sep 17 00:00:00 2001 From: Aetf Date: Tue, 3 Nov 2020 19:02:56 -0500 Subject: [PATCH 4/6] FdoSecrets: handle corner cases in collection dbus names, fix #5279 - Use completeBaseName rather than baseName to ensure nonempty name - Handle two databases have the same name - Cleanup Service::onDatabaseTabOpened logic --- src/fdosecrets/FdoSecretsPlugin.cpp | 2 +- src/fdosecrets/FdoSecretsPlugin.h | 2 +- src/fdosecrets/objects/Collection.cpp | 42 ++++++++++++------------- src/fdosecrets/objects/Collection.h | 6 ++-- src/fdosecrets/objects/Item.cpp | 6 ++-- src/fdosecrets/objects/Prompt.cpp | 20 ++++++------ src/fdosecrets/objects/Service.cpp | 35 ++++++++++++++------- src/fdosecrets/objects/Service.h | 2 +- src/fdosecrets/objects/Session.cpp | 4 +-- tests/gui/TestGuiFdoSecrets.cpp | 45 +++++++++++++++++++++++++++ tests/gui/TestGuiFdoSecrets.h | 3 ++ 11 files changed, 112 insertions(+), 55 deletions(-) diff --git a/src/fdosecrets/FdoSecretsPlugin.cpp b/src/fdosecrets/FdoSecretsPlugin.cpp index 6d44c98c..8004de24 100644 --- a/src/fdosecrets/FdoSecretsPlugin.cpp +++ b/src/fdosecrets/FdoSecretsPlugin.cpp @@ -80,7 +80,7 @@ void FdoSecretsPlugin::updateServiceState() Service* FdoSecretsPlugin::serviceInstance() const { - return m_secretService.get(); + return m_secretService.data(); } DatabaseTabWidget* FdoSecretsPlugin::dbTabs() const diff --git a/src/fdosecrets/FdoSecretsPlugin.h b/src/fdosecrets/FdoSecretsPlugin.h index fdf2aba5..28233460 100644 --- a/src/fdosecrets/FdoSecretsPlugin.h +++ b/src/fdosecrets/FdoSecretsPlugin.h @@ -93,7 +93,7 @@ signals: private: QPointer m_dbTabs; - std::unique_ptr m_secretService; + QSharedPointer m_secretService; }; #endif // KEEPASSXC_FDOSECRETSPLUGIN_H diff --git a/src/fdosecrets/objects/Collection.cpp b/src/fdosecrets/objects/Collection.cpp index a5443fde..448080cb 100644 --- a/src/fdosecrets/objects/Collection.cpp +++ b/src/fdosecrets/objects/Collection.cpp @@ -36,15 +36,7 @@ namespace FdoSecrets { Collection* Collection::Create(Service* parent, DatabaseWidget* backend) { - std::unique_ptr coll{new Collection(parent, backend)}; - if (!coll->reloadBackend()) { - return nullptr; - } - if (!coll->backend()) { - // no exposed group on this db - return nullptr; - } - return coll.release(); + return new Collection(parent, backend); } Collection::Collection(Service* parent, DatabaseWidget* backend) @@ -83,12 +75,20 @@ namespace FdoSecrets unregisterPrimaryPath(); // make sure we have updated copy of the filepath, which is used to identify the database. - m_backendPath = m_backend->database()->filePath(); + m_backendPath = m_backend->database()->canonicalFilePath(); - auto path = QStringLiteral(DBUS_PATH_TEMPLATE_COLLECTION).arg(p()->objectPath().path(), encodePath(name())); + // 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)) { - service()->plugin()->emitError(tr("Failed to register database on DBus under the name '%1'").arg(name())); - return false; + // 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 @@ -450,18 +450,16 @@ namespace FdoSecrets QString Collection::name() const { - // todo: make sure the name is never empty 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 diff --git a/src/fdosecrets/objects/Collection.h b/src/fdosecrets/objects/Collection.h index 0544fc6f..10180787 100644 --- a/src/fdosecrets/objects/Collection.h +++ b/src/fdosecrets/objects/Collection.h @@ -121,13 +121,13 @@ namespace FdoSecrets // delete the Entry in backend from this collection void doDeleteEntries(QList 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 - bool reloadBackend(); - // calls reloadBackend, delete self when error void reloadBackendOrDelete(); diff --git a/src/fdosecrets/objects/Item.cpp b/src/fdosecrets/objects/Item.cpp index 583b3415..adf4f3d4 100644 --- a/src/fdosecrets/objects/Item.cpp +++ b/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 #include +#include #include #include @@ -50,13 +50,13 @@ namespace FdoSecrets Item* Item::Create(Collection* parent, Entry* backend) { - std::unique_ptr res{new Item(parent, backend)}; + QScopedPointer res{new Item(parent, backend)}; if (!res->registerSelf()) { return nullptr; } - return res.release(); + return res.take(); } Item::Item(Collection* parent, Entry* backend) diff --git a/src/fdosecrets/objects/Prompt.cpp b/src/fdosecrets/objects/Prompt.cpp index 6bbc6284..cde09715 100644 --- a/src/fdosecrets/objects/Prompt.cpp +++ b/src/fdosecrets/objects/Prompt.cpp @@ -78,11 +78,11 @@ namespace FdoSecrets DBusReturn DeleteCollectionPrompt::Create(Service* parent, Collection* coll) { - std::unique_ptr res{new DeleteCollectionPrompt(parent, coll)}; + QScopedPointer res{new DeleteCollectionPrompt(parent, coll)}; if (!res->registerSelf()) { return DBusReturn<>::Error(QDBusError::InvalidObjectPath); } - return res.release(); + return res.take(); } DeleteCollectionPrompt::DeleteCollectionPrompt(Service* parent, Collection* coll) @@ -118,11 +118,11 @@ namespace FdoSecrets DBusReturn CreateCollectionPrompt::Create(Service* parent) { - std::unique_ptr res{new CreateCollectionPrompt(parent)}; + QScopedPointer res{new CreateCollectionPrompt(parent)}; if (!res->registerSelf()) { return DBusReturn<>::Error(QDBusError::InvalidObjectPath); } - return res.release(); + return res.take(); } CreateCollectionPrompt::CreateCollectionPrompt(Service* parent) @@ -163,11 +163,11 @@ namespace FdoSecrets DBusReturn LockCollectionsPrompt::Create(Service* parent, const QList& colls) { - std::unique_ptr res{new LockCollectionsPrompt(parent, colls)}; + QScopedPointer res{new LockCollectionsPrompt(parent, colls)}; if (!res->registerSelf()) { return DBusReturn<>::Error(QDBusError::InvalidObjectPath); } - return res.release(); + return res.take(); } LockCollectionsPrompt::LockCollectionsPrompt(Service* parent, const QList& colls) @@ -215,11 +215,11 @@ namespace FdoSecrets DBusReturn UnlockCollectionsPrompt::Create(Service* parent, const QList& coll) { - std::unique_ptr res{new UnlockCollectionsPrompt(parent, coll)}; + QScopedPointer res{new UnlockCollectionsPrompt(parent, coll)}; if (!res->registerSelf()) { return DBusReturn<>::Error(QDBusError::InvalidObjectPath); } - return res.release(); + return res.take(); } UnlockCollectionsPrompt::UnlockCollectionsPrompt(Service* parent, const QList& colls) @@ -291,11 +291,11 @@ namespace FdoSecrets DBusReturn DeleteItemPrompt::Create(Service* parent, Item* item) { - std::unique_ptr res{new DeleteItemPrompt(parent, item)}; + QScopedPointer res{new DeleteItemPrompt(parent, item)}; if (!res->registerSelf()) { return DBusReturn<>::Error(QDBusError::InvalidObjectPath); } - return res.release(); + return res.take(); } DeleteItemPrompt::DeleteItemPrompt(Service* parent, Item* item) diff --git a/src/fdosecrets/objects/Service.cpp b/src/fdosecrets/objects/Service.cpp index a3b4eb19..591da173 100644 --- a/src/fdosecrets/objects/Service.cpp +++ b/src/fdosecrets/objects/Service.cpp @@ -30,8 +30,7 @@ #include #include #include - -#include +#include namespace { @@ -40,9 +39,9 @@ namespace namespace FdoSecrets { - std::unique_ptr Service::Create(FdoSecretsPlugin* plugin, QPointer dbTabs) + QSharedPointer Service::Create(FdoSecretsPlugin* plugin, QPointer dbTabs) { - std::unique_ptr res{new Service(plugin, std::move(dbTabs))}; + QSharedPointer res{new Service(plugin, std::move(dbTabs))}; if (!res->initialize()) { return {}; } @@ -121,21 +120,23 @@ namespace FdoSecrets auto coll = Collection::Create(this, dbWidget); if (!coll) { - // The creation may fail if the database is not exposed. - // This is okay, because we monitor the expose settings above return; } m_collections << coll; m_dbToCollection[dbWidget] = coll; + // 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 @@ -150,14 +151,24 @@ namespace FdoSecrets } }); - // relay signals + // actual load, must after updates to m_collections, because the reload may trigger + // another onDatabaseTabOpen, and m_collections will be used to prevent recursion. + if (!coll->reloadBackend()) { + // error in dbus + return; + } + 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]() { - m_collections.removeAll(coll); - m_dbToCollection.remove(coll->backend()); emit collectionDeleted(coll); }); - if (emitSignal) { emit collectionCreated(coll); } diff --git a/src/fdosecrets/objects/Service.h b/src/fdosecrets/objects/Service.h index 680df711..c0d39191 100644 --- a/src/fdosecrets/objects/Service.h +++ b/src/fdosecrets/objects/Service.h @@ -57,7 +57,7 @@ namespace FdoSecrets * This may be caused by * - failed initialization */ - static std::unique_ptr Create(FdoSecretsPlugin* plugin, QPointer dbTabs); + static QSharedPointer Create(FdoSecretsPlugin* plugin, QPointer dbTabs); ~Service() override; DBusReturn openSession(const QString& algorithm, const QVariant& input, Session*& result); diff --git a/src/fdosecrets/objects/Session.cpp b/src/fdosecrets/objects/Session.cpp index 8a4d6939..0c643f2f 100644 --- a/src/fdosecrets/objects/Session.cpp +++ b/src/fdosecrets/objects/Session.cpp @@ -28,13 +28,13 @@ namespace FdoSecrets Session* Session::Create(std::unique_ptr&& cipher, const QString& peer, Service* parent) { - std::unique_ptr res{new Session(std::move(cipher), peer, parent)}; + QScopedPointer res{new Session(std::move(cipher), peer, parent)}; if (!res->registerSelf()) { return nullptr; } - return res.release(); + return res.take(); } Session::Session(std::unique_ptr&& cipher, const QString& peer, Service* parent) diff --git a/tests/gui/TestGuiFdoSecrets.cpp b/tests/gui/TestGuiFdoSecrets.cpp index af8f32f3..08840088 100644 --- a/tests/gui/TestGuiFdoSecrets.cpp +++ b/tests/gui/TestGuiFdoSecrets.cpp @@ -48,6 +48,7 @@ #include #include #include +#include #include #include @@ -761,6 +762,50 @@ void TestGuiFdoSecrets::testCollectionDelete() } } +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(); diff --git a/tests/gui/TestGuiFdoSecrets.h b/tests/gui/TestGuiFdoSecrets.h index c4ed108e..84f7147e 100644 --- a/tests/gui/TestGuiFdoSecrets.h +++ b/tests/gui/TestGuiFdoSecrets.h @@ -84,6 +84,9 @@ private slots: void testExposeSubgroup(); void testModifyingExposedGroup(); + void testHiddenFilename(); + void testDuplicateName(); + protected slots: void createDatabaseCallback(); From 7f85eb77aa3a4aa41dd0985485471135164950e3 Mon Sep 17 00:00:00 2001 From: Aetf Date: Tue, 3 Nov 2020 19:11:37 -0500 Subject: [PATCH 5/6] FdoSecrets: code formatting --- src/fdosecrets/objects/Collection.cpp | 3 ++- src/fdosecrets/objects/Collection.h | 1 + src/fdosecrets/objects/Item.h | 1 + src/fdosecrets/objects/Prompt.cpp | 6 ++++-- src/fdosecrets/objects/Prompt.h | 5 +++++ src/fdosecrets/objects/Service.cpp | 30 +++++++++++++-------------- src/fdosecrets/objects/Service.h | 1 + src/fdosecrets/objects/Session.h | 1 + 8 files changed, 30 insertions(+), 18 deletions(-) diff --git a/src/fdosecrets/objects/Collection.cpp b/src/fdosecrets/objects/Collection.cpp index 448080cb..0f856d87 100644 --- a/src/fdosecrets/objects/Collection.cpp +++ b/src/fdosecrets/objects/Collection.cpp @@ -415,7 +415,8 @@ namespace FdoSecrets emit aliasAboutToAdd(alias); - bool ok = registerWithPath(QStringLiteral(DBUS_PATH_TEMPLATE_ALIAS).arg(p()->objectPath().path(), alias), false); + bool ok = + registerWithPath(QStringLiteral(DBUS_PATH_TEMPLATE_ALIAS).arg(p()->objectPath().path(), alias), false); if (ok) { m_aliases.insert(alias); emit aliasAdded(alias); diff --git a/src/fdosecrets/objects/Collection.h b/src/fdosecrets/objects/Collection.h index 10180787..80940d5a 100644 --- a/src/fdosecrets/objects/Collection.h +++ b/src/fdosecrets/objects/Collection.h @@ -41,6 +41,7 @@ namespace FdoSecrets Q_OBJECT explicit Collection(Service* parent, DatabaseWidget* backend); + public: /** * @brief Create a new instance of `Collection` diff --git a/src/fdosecrets/objects/Item.h b/src/fdosecrets/objects/Item.h index 97077970..8c753a3d 100644 --- a/src/fdosecrets/objects/Item.h +++ b/src/fdosecrets/objects/Item.h @@ -43,6 +43,7 @@ namespace FdoSecrets Q_OBJECT explicit Item(Collection* parent, Entry* backend); + public: /** * @brief Create a new instance of `Item`. diff --git a/src/fdosecrets/objects/Prompt.cpp b/src/fdosecrets/objects/Prompt.cpp index cde09715..efed6317 100644 --- a/src/fdosecrets/objects/Prompt.cpp +++ b/src/fdosecrets/objects/Prompt.cpp @@ -41,7 +41,8 @@ namespace FdoSecrets bool PromptBase::registerSelf() { - auto path = QStringLiteral(DBUS_PATH_TEMPLATE_PROMPT).arg(p()->objectPath().path(), Tools::uuidToHex(QUuid::createUuid())); + 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)); @@ -213,7 +214,8 @@ namespace FdoSecrets return {}; } - DBusReturn UnlockCollectionsPrompt::Create(Service* parent, const QList& coll) + DBusReturn UnlockCollectionsPrompt::Create(Service* parent, + const QList& coll) { QScopedPointer res{new UnlockCollectionsPrompt(parent, coll)}; if (!res->registerSelf()) { diff --git a/src/fdosecrets/objects/Prompt.h b/src/fdosecrets/objects/Prompt.h index e72f0512..9a972567 100644 --- a/src/fdosecrets/objects/Prompt.h +++ b/src/fdosecrets/objects/Prompt.h @@ -58,6 +58,7 @@ namespace FdoSecrets Q_OBJECT explicit DeleteCollectionPrompt(Service* parent, Collection* coll); + public: static DBusReturn Create(Service* parent, Collection* coll); @@ -72,6 +73,7 @@ namespace FdoSecrets Q_OBJECT explicit CreateCollectionPrompt(Service* parent); + public: static DBusReturn Create(Service* parent); @@ -87,6 +89,7 @@ namespace FdoSecrets Q_OBJECT explicit LockCollectionsPrompt(Service* parent, const QList& colls); + public: static DBusReturn Create(Service* parent, const QList& colls); @@ -103,6 +106,7 @@ namespace FdoSecrets Q_OBJECT explicit UnlockCollectionsPrompt(Service* parent, const QList& coll); + public: static DBusReturn Create(Service* parent, const QList& coll); @@ -124,6 +128,7 @@ namespace FdoSecrets Q_OBJECT explicit DeleteItemPrompt(Service* parent, Item* item); + public: static DBusReturn Create(Service* parent, Item* item); diff --git a/src/fdosecrets/objects/Service.cpp b/src/fdosecrets/objects/Service.cpp index 591da173..957203d8 100644 --- a/src/fdosecrets/objects/Service.cpp +++ b/src/fdosecrets/objects/Service.cpp @@ -68,8 +68,9 @@ namespace FdoSecrets bool Service::initialize() { if (!QDBusConnection::sessionBus().registerService(QStringLiteral(DBUS_SERVICE_SECRET))) { - plugin()->emitError(tr("Failed to register DBus service at %1.
").arg(QLatin1String(DBUS_SERVICE_SECRET)) - + m_plugin->reportExistingService()); + plugin()->emitError( + tr("Failed to register DBus service at %1.
").arg(QLatin1String(DBUS_SERVICE_SECRET)) + + m_plugin->reportExistingService()); return false; } @@ -80,10 +81,8 @@ namespace FdoSecrets // Connect to service unregistered signal m_serviceWatcher.reset(new QDBusServiceWatcher()); - connect(m_serviceWatcher.get(), - &QDBusServiceWatcher::serviceUnregistered, - this, - &Service::dbusServiceUnregistered); + connect( + m_serviceWatcher.get(), &QDBusServiceWatcher::serviceUnregistered, this, &Service::dbusServiceUnregistered); m_serviceWatcher->setConnection(QDBusConnection::sessionBus()); @@ -166,9 +165,7 @@ namespace FdoSecrets // 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); - }); + connect(coll, &Collection::collectionAboutToDelete, this, [this, coll]() { emit collectionDeleted(coll); }); if (emitSignal) { emit collectionCreated(coll); } @@ -275,12 +272,15 @@ namespace FdoSecrets // collection will be created when the prompt completes. // once it's done, we set additional properties on the collection - connect(cp.value(), &CreateCollectionPrompt::collectionCreated, cp.value(), [alias, properties](Collection* coll) { - coll->setProperties(properties).okOrDie(); - if (!alias.isEmpty()) { - coll->addAlias(alias).okOrDie(); - } - }); + connect(cp.value(), + &CreateCollectionPrompt::collectionCreated, + cp.value(), + [alias, properties](Collection* coll) { + coll->setProperties(properties).okOrDie(); + if (!alias.isEmpty()) { + coll->addAlias(alias).okOrDie(); + } + }); } return collection; } diff --git a/src/fdosecrets/objects/Service.h b/src/fdosecrets/objects/Service.h index c0d39191..5b1ff5ac 100644 --- a/src/fdosecrets/objects/Service.h +++ b/src/fdosecrets/objects/Service.h @@ -50,6 +50,7 @@ namespace FdoSecrets Q_OBJECT explicit Service(FdoSecretsPlugin* plugin, QPointer dbTabs); + public: /** * @brief Create a new instance of `Service`. Its parent is set to `null` diff --git a/src/fdosecrets/objects/Session.h b/src/fdosecrets/objects/Session.h index 5722895a..3bb6ea25 100644 --- a/src/fdosecrets/objects/Session.h +++ b/src/fdosecrets/objects/Session.h @@ -39,6 +39,7 @@ namespace FdoSecrets Q_OBJECT explicit Session(std::unique_ptr&& cipher, const QString& peer, Service* parent); + public: static std::unique_ptr CreateCiphers(const QString& peer, const QString& algorithm, From 9f4118974dca3335b683e79b172fd94b48afdeda Mon Sep 17 00:00:00 2001 From: Aetf Date: Fri, 13 Nov 2020 17:14:03 -0500 Subject: [PATCH 6/6] FdoSecrets: fix signal connections --- .../objects/adaptors/CollectionAdaptor.cpp | 16 ++- src/fdosecrets/objects/adaptors/DBusAdaptor.h | 2 +- .../objects/adaptors/PromptAdaptor.cpp | 3 +- .../objects/adaptors/ServiceAdaptor.cpp | 7 +- tests/gui/TestGuiFdoSecrets.cpp | 109 +++++++++++------- 5 files changed, 87 insertions(+), 50 deletions(-) diff --git a/src/fdosecrets/objects/adaptors/CollectionAdaptor.cpp b/src/fdosecrets/objects/adaptors/CollectionAdaptor.cpp index 77eb3b41..275145b4 100644 --- a/src/fdosecrets/objects/adaptors/CollectionAdaptor.cpp +++ b/src/fdosecrets/objects/adaptors/CollectionAdaptor.cpp @@ -27,12 +27,16 @@ namespace FdoSecrets CollectionAdaptor::CollectionAdaptor(Collection* parent) : DBusAdaptor(parent) { - connect( - p(), &Collection::itemCreated, this, [this](const Item* item) { emit ItemCreated(objectPathSafe(item)); }); - connect( - p(), &Collection::itemDeleted, this, [this](const Item* item) { emit ItemDeleted(objectPathSafe(item)); }); - connect( - p(), &Collection::itemChanged, this, [this](const Item* item) { emit ItemChanged(objectPathSafe(item)); }); + // p() isn't ready yet as this is called in Parent's constructor + connect(parent, &Collection::itemCreated, this, [this](const Item* item) { + emit ItemCreated(objectPathSafe(item)); + }); + connect(parent, &Collection::itemDeleted, this, [this](const Item* item) { + emit ItemDeleted(objectPathSafe(item)); + }); + connect(parent, &Collection::itemChanged, this, [this](const Item* item) { + emit ItemChanged(objectPathSafe(item)); + }); } const QList CollectionAdaptor::items() const diff --git a/src/fdosecrets/objects/adaptors/DBusAdaptor.h b/src/fdosecrets/objects/adaptors/DBusAdaptor.h index bd70ab60..93bbc72f 100644 --- a/src/fdosecrets/objects/adaptors/DBusAdaptor.h +++ b/src/fdosecrets/objects/adaptors/DBusAdaptor.h @@ -32,7 +32,7 @@ namespace FdoSecrets template class DBusAdaptor : public QDBusAbstractAdaptor { public: - explicit DBusAdaptor(QObject* parent = nullptr) + explicit DBusAdaptor(Parent* parent = nullptr) : QDBusAbstractAdaptor(parent) { } diff --git a/src/fdosecrets/objects/adaptors/PromptAdaptor.cpp b/src/fdosecrets/objects/adaptors/PromptAdaptor.cpp index bcc1e271..ff8a945c 100644 --- a/src/fdosecrets/objects/adaptors/PromptAdaptor.cpp +++ b/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{}; diff --git a/src/fdosecrets/objects/adaptors/ServiceAdaptor.cpp b/src/fdosecrets/objects/adaptors/ServiceAdaptor.cpp index a260c492..cacf9a99 100644 --- a/src/fdosecrets/objects/adaptors/ServiceAdaptor.cpp +++ b/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)); }); } diff --git a/tests/gui/TestGuiFdoSecrets.cpp b/tests/gui/TestGuiFdoSecrets.cpp index 08840088..eb8192d5 100644 --- a/tests/gui/TestGuiFdoSecrets.cpp +++ b/tests/gui/TestGuiFdoSecrets.cpp @@ -174,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(); - qRegisterMetaType(); - QVERIFY(Crypto::init()); Config::createTempFileInstance(); config()->set(Config::AutoSaveAfterEveryChange, false); @@ -458,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; @@ -472,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 @@ -510,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>(), {coll->objectPath()}); + QCOMPARE(args.at(1).value().variant().value>(), {coll->objectPath()}); } QCOMPARE(spyCollectionCreated.count(), 0); QCOMPARE(spyCollectionChanged.count(), 1); { auto args = spyCollectionChanged.takeFirst(); QCOMPARE(args.size(), 1); - QCOMPARE(args.at(0).value(), coll.data()); + QCOMPARE(args.at(0).value(), coll->objectPath()); } QCOMPARE(spyCollectionDeleted.count(), 0); } @@ -529,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 @@ -543,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 @@ -564,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 @@ -578,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>(), {coll->objectPath()}); + QCOMPARE(args.at(1).value().variant().value>(), {coll->objectPath()}); } QCOMPARE(spyCollectionCreated.count(), 0); @@ -586,7 +578,7 @@ void TestGuiFdoSecrets::testServiceLock() { auto args = spyCollectionChanged.takeFirst(); QCOMPARE(args.size(), 1); - QCOMPARE(args.at(0).value(), coll.data()); + QCOMPARE(args.at(0).value(), coll->objectPath()); } QCOMPARE(spyCollectionDeleted.count(), 0); @@ -642,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 @@ -664,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())); @@ -675,7 +667,8 @@ void TestGuiFdoSecrets::testCollectionCreate() auto args = spyPromptCompleted.takeFirst(); QCOMPARE(args.size(), 2); QCOMPARE(args.at(0).toBool(), false); - auto coll = FdoSecrets::pathToObject(args.at(1).value()); + auto coll = + FdoSecrets::pathToObject(args.at(1).value().variant().value()); QVERIFY(coll); QCOMPARE(coll->backend()->database()->metadata()->name(), QStringLiteral("Test NewDB")); @@ -684,7 +677,7 @@ void TestGuiFdoSecrets::testCollectionCreate() { args = spyCollectionCreated.takeFirst(); QCOMPARE(args.size(), 1); - QCOMPARE(args.at(0).value(), coll); + QCOMPARE(args.at(0).value(), coll->objectPath()); } } } @@ -723,18 +716,16 @@ void TestGuiFdoSecrets::testCollectionDelete() QVERIFY(service); auto coll = getDefaultCollection(service); QVERIFY(coll); - // closing the tab calls coll->deleteLater() - // but deleteLater is not processed in QApplication::processEvent - // see https://doc.qt.io/qt-5/qcoreapplication.html#processEvents - auto rawColl = coll.data(); + // save the path which will be gone after the deletion. + auto collPath = coll->objectPath(); - 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 @@ -752,13 +743,13 @@ 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().variant().toString(), QStringLiteral("")); QCOMPARE(spyCollectionDeleted.count(), 1); { args = spyCollectionDeleted.takeFirst(); QCOMPARE(args.size(), 1); - QCOMPARE(args.at(0).value(), rawColl); + QCOMPARE(args.at(0).value(), collPath); } } @@ -815,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"}, @@ -824,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(), item->objectPath()); + } + // attributes { CHECKED_DBUS_LOCAL_CALL(actual, item->attributes()); @@ -888,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 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(), 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 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(), 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(), item4->objectPath()); + } } } @@ -994,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 @@ -1025,7 +1056,7 @@ void TestGuiFdoSecrets::testItemDelete() { args = spyItemDeleted.takeFirst(); QCOMPARE(args.size(), 1); - QCOMPARE(args.at(0).value(), rawItem); + QCOMPARE(args.at(0).value(), itemPath); } }