From 8dee707cf70396d88ad47a6004ef29143e058739 Mon Sep 17 00:00:00 2001 From: Aetf Date: Tue, 3 Nov 2020 13:06:04 -0500 Subject: [PATCH] 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 | 57 +++++++++++++-------------- 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, 93 insertions(+), 69 deletions(-) diff --git a/src/fdosecrets/FdoSecretsPlugin.cpp b/src/fdosecrets/FdoSecretsPlugin.cpp index e75a77aedc..6d44c98c9c 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 90d6d9e827..a5443fde47 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(); + Q_ASSERT(m_backend); - unregisterCurrentPath(); - m_registered = false; + // delete all items + // this has to be done because the backend is actually still there, just we don't expose them + // NOTE: Do NOT use a for loop, because Item::doDelete will remove itself from m_items. + while (!m_items.isEmpty()) { + m_items.first()->doDelete(); } - - Q_ASSERT(m_backend); + cleanupConnections(); + unregisterPrimaryPath(); // make sure we have updated copy of the filepath, which is used to identify the database. m_backendPath = m_backend->database()->filePath(); - // the database may not have a name (derived from filePath) yet, which may happen if it's newly created. - // defer the registration to next time a file path change happens. - if (!name().isEmpty()) { - 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 5f06cc3324..0544fc6f39 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 c53baa72e2..fb7533c1ca 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 eeb505072a..d51642a86c 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 038b9a922d..583b341572 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 746850e79b..9707797068 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 b20be86f15..6bbc628467 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 10939dcbcd..e72f0512c3 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 2577b476e8..a3b4eb1966 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 cf27b27734..680df71184 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 26a3c0cf8b..8a4d6939bd 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 58b971fc48..5722895adc 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