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/objects/Collection.cpp

@@ -37,13 +37,6 @@ 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();
     }
 
@@ -83,12 +76,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 +451,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/Service.cpp

@@ -121,21 +121,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 +152,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);
         }

tests/gui/TestGuiFdoSecrets.cpp

@@ -48,6 +48,7 @@
 #include <QLineEdit>
 #include <QPointer>
 #include <QSignalSpy>
+#include <QTemporaryDir>
 #include <QXmlStreamReader>
 
 #include <memory>
@@ -761,6 +762,49 @@ 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(), ".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
+    CHECKED_DBUS_LOCAL_CALL(colls, service->collections());
+    QCOMPARE(colls.size(), 2);
+    QCOMPARE(colls[0]->objectPath().path(), "/org/freedesktop/secrets/collection/KeePassXC");
+    QVERIFY(colls[1]->objectPath().path() != "/org/freedesktop/secrets/collection/KeePassXC");
+}
+
 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();