diff --git a/src/fdosecrets/FdoSecretsPlugin.cpp b/src/fdosecrets/FdoSecretsPlugin.cpp index 96d76e345..8004de246 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; } @@ -107,6 +101,12 @@ 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)); + qDebug() << msg; +} + QString FdoSecretsPlugin::reportExistingService() const { auto pidStr = tr("Unknown", "Unknown PID"); diff --git a/src/fdosecrets/FdoSecretsPlugin.h b/src/fdosecrets/FdoSecretsPlugin.h index ec57fec82..282334600 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; + QSharedPointer m_secretService; }; #endif // KEEPASSXC_FDOSECRETSPLUGIN_H diff --git a/src/fdosecrets/objects/Collection.cpp b/src/fdosecrets/objects/Collection.cpp index c0bb8ff34..0f856d87f 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" @@ -24,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" @@ -34,16 +34,19 @@ namespace FdoSecrets { + Collection* Collection::Create(Service* parent, DatabaseWidget* backend) + { + return new Collection(parent, backend); + } 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); @@ -56,37 +59,36 @@ namespace FdoSecrets } emit doneUnlockCollection(accepted); }); - - reloadBackend(); } - void Collection::reloadBackend() + 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); - // make sure we have updated copy of the filepath, which is used to identify the database. - m_backendPath = m_backend->database()->filePath(); + // 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(); - // 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)); - m_registered = true; + // make sure we have updated copy of the filepath, which is used to identify the database. + m_backendPath = m_backend->database()->canonicalFilePath(); + + // 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)) { + // 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 @@ -95,6 +97,15 @@ namespace FdoSecrets } else { cleanupConnections(); } + + return true; + } + + void Collection::reloadBackendOrDelete() + { + if (!reloadBackend()) { + doDelete(); + } } DBusReturn Collection::ensureBackend() const @@ -197,7 +208,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({}); @@ -311,12 +326,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; } } @@ -400,11 +415,13 @@ 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); + } else { + return DBusReturn<>::Error(QDBusError::InvalidObjectPath); } return {}; @@ -435,16 +452,15 @@ namespace FdoSecrets QString Collection::name() const { 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 @@ -466,7 +482,7 @@ namespace FdoSecrets void Collection::populateContents() { - if (!m_registered) { + if (!m_backend) { return; } @@ -558,7 +574,7 @@ namespace FdoSecrets return; } - auto item = new Item(this, entry); + auto item = Item::Create(this, entry); m_items << item; m_entryToItem[entry] = item; @@ -625,7 +641,7 @@ namespace FdoSecrets emit collectionAboutToDelete(); - unregisterCurrentPath(); + unregisterPrimaryPath(); // remove alias manually to trigger signal for (const auto& a : aliases()) { @@ -643,6 +659,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 da6dcb389..80940d5a7 100644 --- a/src/fdosecrets/objects/Collection.h +++ b/src/fdosecrets/objects/Collection.h @@ -36,12 +36,24 @@ namespace FdoSecrets class Item; class PromptBase; class Service; - class Collection : public DBusObject + class Collection : public DBusObjectHelper { 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; DBusReturn label() const; @@ -101,7 +113,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 @@ -110,11 +122,15 @@ 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 - void reloadBackend(); + + // calls reloadBackend, delete self when error + void reloadBackendOrDelete(); private: friend class DeleteCollectionPrompt; @@ -154,8 +170,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 8bf1ae4e5..fb7533c1c 100644 --- a/src/fdosecrets/objects/DBusObject.cpp +++ b/src/fdosecrets/objects/DBusObject.cpp @@ -17,31 +17,28 @@ #include "DBusObject.h" -#include #include #include #include #include #include -#include - 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, bool primary) { - 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); + if (primary) { + m_objectPath.setPath(path); + } + + return QDBusConnection::sessionBus().registerObject(path, this); } QString DBusObject::callingPeerName() const @@ -57,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 4cdaf5ced..d51642a86 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: - void 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/DBusTypes.h b/src/fdosecrets/objects/DBusTypes.h index 384a1e6b2..ef1e2276a 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 d58cbcc2c..adf4f3d4c 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 @@ -48,18 +48,36 @@ namespace FdoSecrets constexpr auto FDO_SECRETS_CONTENT_TYPE = "FDO_SECRETS_CONTENT_TYPE"; } // namespace + Item* Item::Create(Collection* parent, Entry* backend) + { + QScopedPointer res{new Item(parent, backend)}; + + if (!res->registerSelf()) { + return nullptr; + } + + return res.take(); + } + Item::Item(Collection* parent, Entry* backend) - : DBusObject(parent) + : DBusObjectHelper(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); + 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) @@ -322,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 99601d950..8c753a3d5 100644 --- a/src/fdosecrets/objects/Item.h +++ b/src/fdosecrets/objects/Item.h @@ -38,12 +38,23 @@ namespace FdoSecrets class Collection; class PromptBase; - class Item : public DBusObject + class Item : public DBusObjectHelper { 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; DBusReturn attributes() const; @@ -90,7 +101,12 @@ namespace FdoSecrets public slots: 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 cb3cb4f48..efed63179 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" @@ -33,15 +34,22 @@ namespace FdoSecrets { PromptBase::PromptBase(Service* parent) - : DBusObject(parent) + : DBusObjectHelper(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); + 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 +77,15 @@ namespace FdoSecrets return {}; } + DBusReturn DeleteCollectionPrompt::Create(Service* parent, Collection* coll) + { + QScopedPointer res{new DeleteCollectionPrompt(parent, coll)}; + if (!res->registerSelf()) { + return DBusReturn<>::Error(QDBusError::InvalidObjectPath); + } + return res.take(); + } + DeleteCollectionPrompt::DeleteCollectionPrompt(Service* parent, Collection* coll) : PromptBase(parent) , m_collection(coll) @@ -100,6 +117,15 @@ namespace FdoSecrets return {}; } + DBusReturn CreateCollectionPrompt::Create(Service* parent) + { + QScopedPointer res{new CreateCollectionPrompt(parent)}; + if (!res->registerSelf()) { + return DBusReturn<>::Error(QDBusError::InvalidObjectPath); + } + return res.take(); + } + CreateCollectionPrompt::CreateCollectionPrompt(Service* parent) : PromptBase(parent) { @@ -136,6 +162,15 @@ namespace FdoSecrets return {}; } + DBusReturn LockCollectionsPrompt::Create(Service* parent, const QList& colls) + { + QScopedPointer res{new LockCollectionsPrompt(parent, colls)}; + if (!res->registerSelf()) { + return DBusReturn<>::Error(QDBusError::InvalidObjectPath); + } + return res.take(); + } + LockCollectionsPrompt::LockCollectionsPrompt(Service* parent, const QList& colls) : PromptBase(parent) { @@ -179,6 +214,16 @@ namespace FdoSecrets return {}; } + DBusReturn UnlockCollectionsPrompt::Create(Service* parent, + const QList& coll) + { + QScopedPointer res{new UnlockCollectionsPrompt(parent, coll)}; + if (!res->registerSelf()) { + return DBusReturn<>::Error(QDBusError::InvalidObjectPath); + } + return res.take(); + } + UnlockCollectionsPrompt::UnlockCollectionsPrompt(Service* parent, const QList& colls) : PromptBase(parent) { @@ -246,6 +291,15 @@ namespace FdoSecrets return {}; } + DBusReturn DeleteItemPrompt::Create(Service* parent, Item* item) + { + QScopedPointer res{new DeleteItemPrompt(parent, item)}; + if (!res->registerSelf()) { + return DBusReturn<>::Error(QDBusError::InvalidObjectPath); + } + return res.take(); + } + 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 683642df5..9a9725675 100644 --- a/src/fdosecrets/objects/Prompt.h +++ b/src/fdosecrets/objects/Prompt.h @@ -32,12 +32,10 @@ namespace FdoSecrets class Service; - class PromptBase : public DBusObject + class PromptBase : public DBusObjectHelper { 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,9 +57,11 @@ 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; private: @@ -69,9 +72,11 @@ 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,9 +87,12 @@ 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,9 +104,12 @@ 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,9 +127,11 @@ 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; private: diff --git a/src/fdosecrets/objects/Service.cpp b/src/fdosecrets/objects/Service.cpp index 892ba68a4..957203d8b 100644 --- a/src/fdosecrets/objects/Service.cpp +++ b/src/fdosecrets/objects/Service.cpp @@ -30,6 +30,7 @@ #include #include #include +#include namespace { @@ -38,13 +39,21 @@ namespace namespace FdoSecrets { + QSharedPointer Service::Create(FdoSecretsPlugin* plugin, QPointer dbTabs) + { + QSharedPointer 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 - : DBusObject(nullptr) + : DBusObjectHelper(nullptr) , m_plugin(plugin) , m_databases(std::move(dbTabs)) - , m_insdieEnsureDefaultAlias(false) + , m_insideEnsureDefaultAlias(false) , m_serviceWatcher(nullptr) { connect( @@ -59,19 +68,21 @@ 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)) - + m_plugin->reportExistingService()); + 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))) { + 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(), - &QDBusServiceWatcher::serviceUnregistered, - this, - &Service::dbusServiceUnregistered); + connect( + m_serviceWatcher.get(), &QDBusServiceWatcher::serviceUnregistered, this, &Service::dbusServiceUnregistered); m_serviceWatcher->setConnection(QDBusConnection::sessionBus()); @@ -99,31 +110,32 @@ 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 = 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) { return; } m_collections << coll; m_dbToCollection[dbWidget] = coll; - // handle alias + // 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 @@ -138,14 +150,22 @@ namespace FdoSecrets } }); - // relay signals - 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); - }); + // 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]() { emit collectionDeleted(coll); }); if (emitSignal) { emit collectionCreated(coll); } @@ -164,11 +184,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) { @@ -176,7 +196,7 @@ namespace FdoSecrets coll->addAlias(DEFAULT_ALIAS).okOrDie(); } - m_insdieEnsureDefaultAlias = false; + m_insideEnsureDefaultAlias = false; } void Service::dbusServiceUnregistered(const QString& service) @@ -184,8 +204,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 +239,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,17 +264,23 @@ 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) { - 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; } @@ -314,7 +344,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 +386,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 4e0eeb0ff..5b1ff5acd 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; @@ -44,14 +45,21 @@ 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 - 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 QSharedPointer Create(FdoSecretsPlugin* plugin, QPointer dbTabs); + ~Service() override; DBusReturn openSession(const QString& algorithm, const QVariant& input, Session*& result); DBusReturn @@ -82,12 +90,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 +133,8 @@ namespace FdoSecrets void onCollectionAliasRemoved(const QString& alias); private: + bool initialize(); + /** * Find collection by alias name * @param alias @@ -156,9 +160,9 @@ namespace FdoSecrets QList m_sessions; QHash m_peerToSession; - bool m_insdieEnsureDefaultAlias; + bool m_insideEnsureDefaultAlias; - 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 4845752a1..0c643f2f1 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" @@ -23,21 +24,40 @@ namespace FdoSecrets { - QHash Session::negoniationState; + QHash Session::negotiationState; + + Session* Session::Create(std::unique_ptr&& cipher, const QString& peer, Service* parent) + { + QScopedPointer res{new Session(std::move(cipher), peer, parent)}; + + if (!res->registerSelf()) { + return nullptr; + } + + return res.take(); + } 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()) { - 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); + 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) { - negoniationState.remove(peer); + negotiationState.remove(peer); } DBusReturn Session::close() @@ -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 472cc6e5a..3bb6ea257 100644 --- a/src/fdosecrets/objects/Session.h +++ b/src/fdosecrets/objects/Session.h @@ -34,18 +34,30 @@ namespace FdoSecrets { class CipherPair; - class Session : public DBusObject + class Session : public DBusObjectHelper { 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, - const QVariant& intpu, + const QVariant& input, QVariant& output, 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 +83,8 @@ namespace FdoSecrets QString id() const; + Service* service() const; + signals: /** * The session is going to be closed @@ -78,12 +92,15 @@ namespace FdoSecrets */ void aboutToClose(); + private: + bool registerSelf(); + private: std::unique_ptr m_cipher; QString m_peer; QUuid m_id; - static QHash negoniationState; + static QHash negotiationState; }; } // namespace FdoSecrets diff --git a/src/fdosecrets/objects/adaptors/CollectionAdaptor.cpp b/src/fdosecrets/objects/adaptors/CollectionAdaptor.cpp index 77eb3b413..275145b44 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 bd70ab60a..93bbc72f0 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 bcc1e271d..ff8a945cd 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 a260c4920..cacf9a994 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 e19e61308..eb8192d5f 100644 --- a/tests/gui/TestGuiFdoSecrets.cpp +++ b/tests/gui/TestGuiFdoSecrets.cpp @@ -48,6 +48,7 @@ #include #include #include +#include #include #include @@ -173,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); @@ -267,7 +260,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()); } @@ -457,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; @@ -471,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 @@ -509,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); } @@ -528,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 @@ -542,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 @@ -563,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 @@ -577,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); @@ -585,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); @@ -641,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 @@ -663,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())); @@ -674,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")); @@ -683,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()); } } } @@ -722,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 @@ -751,16 +743,60 @@ 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); } } +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(); @@ -770,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"}, @@ -779,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()); @@ -843,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()); + } } } @@ -949,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 @@ -980,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); } } @@ -1047,7 +1123,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 8a961eb8e..84f7147e7 100644 --- a/tests/gui/TestGuiFdoSecrets.h +++ b/tests/gui/TestGuiFdoSecrets.h @@ -82,7 +82,10 @@ private slots: void testDefaultAliasAlwaysPresent(); void testExposeSubgroup(); - void testModifiyingExposedGroup(); + void testModifyingExposedGroup(); + + void testHiddenFilename(); + void testDuplicateName(); protected slots: void createDatabaseCallback();