From a651d7049dcd69cab6ecaecda969767fb827b303 Mon Sep 17 00:00:00 2001 From: Aetf Date: Tue, 3 Nov 2020 19:02:56 -0500 Subject: [PATCH] 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 6d44c98c9..8004de246 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 fdf2aba53..282334600 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 a5443fde4..448080cbc 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 0544fc6f3..101807873 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 583b34157..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 @@ -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 6bbc62846..cde09715f 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 a3b4eb196..591da1737 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 680df7118..c0d39191a 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 8a4d6939b..0c643f2f1 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 af8f32f3a..08840088f 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 c4ed108ee..84f7147e7 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();