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