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

@@ -80,7 +80,7 @@ void FdoSecretsPlugin::updateServiceState()
 
 Service* FdoSecretsPlugin::serviceInstance() const
 {
-    return m_secretService.get();
+    return m_secretService.data();
 }
 
 DatabaseTabWidget* FdoSecretsPlugin::dbTabs() const

src/fdosecrets/FdoSecretsPlugin.h

@@ -93,7 +93,7 @@ signals:
 
 private:
     QPointer<DatabaseTabWidget> m_dbTabs;
-    std::unique_ptr<FdoSecrets::Service> m_secretService;
+    QSharedPointer<FdoSecrets::Service> m_secretService;
 };
 
 #endif // KEEPASSXC_FDOSECRETSPLUGIN_H

src/fdosecrets/objects/Collection.cpp

@@ -36,15 +36,7 @@ namespace FdoSecrets
 {
     Collection* Collection::Create(Service* parent, DatabaseWidget* backend)
     {
-        std::unique_ptr<Collection> 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,13 +75,21 @@ 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()));
+            // 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
         if (!backendLocked()) {
@@ -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

src/fdosecrets/objects/Collection.h

@@ -121,13 +121,13 @@ namespace FdoSecrets
         // delete the Entry in backend from this collection
         void doDeleteEntries(QList<Entry*> 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();
 

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 <QMimeDatabase>
 #include <QRegularExpression>
+#include <QScopedPointer>
 #include <QSet>
 #include <QTextCodec>
 
@@ -50,13 +50,13 @@ namespace FdoSecrets
 
     Item* Item::Create(Collection* parent, Entry* backend)
     {
-        std::unique_ptr<Item> res{new Item(parent, backend)};
+        QScopedPointer<Item> res{new Item(parent, backend)};
 
         if (!res->registerSelf()) {
             return nullptr;
         }
 
-        return res.release();
+        return res.take();
     }
 
     Item::Item(Collection* parent, Entry* backend)

src/fdosecrets/objects/Prompt.cpp

@@ -78,11 +78,11 @@ namespace FdoSecrets
 
     DBusReturn<DeleteCollectionPrompt*> DeleteCollectionPrompt::Create(Service* parent, Collection* coll)
     {
-        std::unique_ptr<DeleteCollectionPrompt> res{new DeleteCollectionPrompt(parent, coll)};
+        QScopedPointer<DeleteCollectionPrompt> 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*> CreateCollectionPrompt::Create(Service* parent)
     {
-        std::unique_ptr<CreateCollectionPrompt> res{new CreateCollectionPrompt(parent)};
+        QScopedPointer<CreateCollectionPrompt> 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*> LockCollectionsPrompt::Create(Service* parent, const QList<Collection*>& colls)
     {
-        std::unique_ptr<LockCollectionsPrompt> res{new LockCollectionsPrompt(parent, colls)};
+        QScopedPointer<LockCollectionsPrompt> 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<Collection*>& colls)
@@ -215,11 +215,11 @@ namespace FdoSecrets
 
     DBusReturn<UnlockCollectionsPrompt*> UnlockCollectionsPrompt::Create(Service* parent, const QList<Collection*>& coll)
     {
-        std::unique_ptr<UnlockCollectionsPrompt> res{new UnlockCollectionsPrompt(parent, coll)};
+        QScopedPointer<UnlockCollectionsPrompt> 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<Collection*>& colls)
@@ -291,11 +291,11 @@ namespace FdoSecrets
 
     DBusReturn<DeleteItemPrompt*> DeleteItemPrompt::Create(Service* parent, Item* item)
     {
-        std::unique_ptr<DeleteItemPrompt> res{new DeleteItemPrompt(parent, item)};
+        QScopedPointer<DeleteItemPrompt> 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)

src/fdosecrets/objects/Service.cpp

@@ -30,8 +30,7 @@
 #include <QDBusConnection>
 #include <QDBusServiceWatcher>
 #include <QDebug>
-
-#include <utility>
+#include <QSharedPointer>
 
 namespace
 {
@@ -40,9 +39,9 @@ namespace
 
 namespace FdoSecrets
 {
-    std::unique_ptr<Service> Service::Create(FdoSecretsPlugin* plugin, QPointer<DatabaseTabWidget> dbTabs)
+    QSharedPointer<Service> Service::Create(FdoSecretsPlugin* plugin, QPointer<DatabaseTabWidget> dbTabs)
     {
-        std::unique_ptr<Service> res{new Service(plugin, std::move(dbTabs))};
+        QSharedPointer<Service> 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);
         }

src/fdosecrets/objects/Service.h

@@ -57,7 +57,7 @@ namespace FdoSecrets
          * This may be caused by
          *   - failed initialization
          */
-        static std::unique_ptr<Service> Create(FdoSecretsPlugin* plugin, QPointer<DatabaseTabWidget> dbTabs);
+        static QSharedPointer<Service> Create(FdoSecretsPlugin* plugin, QPointer<DatabaseTabWidget> dbTabs);
         ~Service() override;
 
         DBusReturn<QVariant> openSession(const QString& algorithm, const QVariant& input, Session*& result);

src/fdosecrets/objects/Session.cpp

@@ -28,13 +28,13 @@ namespace FdoSecrets
 
     Session* Session::Create(std::unique_ptr<CipherPair>&& cipher, const QString& peer, Service* parent)
     {
-        std::unique_ptr<Session> res{new Session(std::move(cipher), peer, parent)};
+        QScopedPointer<Session> res{new Session(std::move(cipher), peer, parent)};
 
         if (!res->registerSelf()) {
             return nullptr;
         }
 
-        return res.release();
+        return res.take();
     }
 
     Session::Session(std::unique_ptr<CipherPair>&& cipher, const QString& peer, Service* parent)

tests/gui/TestGuiFdoSecrets.cpp

@@ -48,6 +48,7 @@
 #include <QLineEdit>
 #include <QPointer>
 #include <QSignalSpy>
+#include <QTemporaryDir>
 #include <QXmlStreamReader>
 
 #include <memory>
@@ -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();

tests/gui/TestGuiFdoSecrets.h

@@ -84,6 +84,9 @@ private slots:
     void testExposeSubgroup();
     void testModifyingExposedGroup();
 
+    void testHiddenFilename();
+    void testDuplicateName();
+
 protected slots:
     void createDatabaseCallback();