Minor changes to Group API to make it more explicit

* Include check for group as recycle bin directly into the Group::isRecycled() function * Return the original root group from Database::setRootGroup(...) to force memory management transfer
keepassxreboot · Mar 9, 2024 · dae2f7a · dae2f7a
1 parent 8ee8339
commit dae2f7a
Show file tree

Hide file tree

Showing 14 changed files with 46 additions and 70 deletions.
diff --git a/src/core/Database.cpp b/src/core/Database.cpp
@@ -66,9 +66,9 @@ Database::Database()
     // block modified signal and set root group
     setEmitModified(false);
 
-    setRootGroup(new Group());
-    rootGroup()->setUuid(QUuid::createUuid());
-    rootGroup()->setName(tr("Passwords", "Root group name"));
+    // Note: oldGroup is nullptr but need to respect return value capture
+    auto oldGroup = setRootGroup(new Group());
+    Q_UNUSED(oldGroup)
 
     m_modified = false;
     setEmitModified(true);
@@ -479,9 +479,8 @@ void Database::releaseData()
     m_data.clear();
     m_metadata->clear();
 
-    auto oldGroup = rootGroup();
-    setRootGroup(new Group());
-    // explicitly delete old group, otherwise it is only deleted when the database object is destructed
+    // Reset and delete the root group
+    auto oldGroup = setRootGroup(new Group());
     delete oldGroup;
 
     m_fileWatcher->stop();
@@ -557,23 +556,30 @@ const Group* Database::rootGroup() const
     return m_rootGroup;
 }
 
-/**
- * Sets group as the root group and takes ownership of it.
- * Warning: Be careful when calling this method as it doesn't
- *          emit any notifications so e.g. models aren't updated.
- *          The caller is responsible for cleaning up the previous
-            root group.
+/* Set the root group of the database and return
+ * the old root group. It is the responsibility
+ * of the calling function to dispose of the old
+ * root group.
  */
-void Database::setRootGroup(Group* group)
+Group* Database::setRootGroup(Group* group)
 {
     Q_ASSERT(group);
 
     if (isInitialized() && isModified()) {
         emit databaseDiscarded();
     }
 
+    auto oldRoot = m_rootGroup;
     m_rootGroup = group;
     m_rootGroup->setParent(this);
+
+    // Initialize the root group if not done already
+    if (m_rootGroup->uuid().isNull()) {
+        m_rootGroup->setUuid(QUuid::createUuid());
+        m_rootGroup->setName(tr("Passwords", "Root group name"));
+    }
+
+    return oldRoot;
 }
 
 Metadata* Database::metadata()

diff --git a/src/core/Database.h b/src/core/Database.h
@@ -111,7 +111,7 @@ class Database : public ModifiableObject
     const Metadata* metadata() const;
     Group* rootGroup();
     const Group* rootGroup() const;
-    void setRootGroup(Group* group);
+    Q_REQUIRED_RESULT Group* setRootGroup(Group* group);
     QVariantMap& publicCustomData();
     const QVariantMap& publicCustomData() const;
     void setPublicCustomData(const QVariantMap& customData);

diff --git a/src/core/Entry.cpp b/src/core/Entry.cpp
@@ -463,7 +463,7 @@ bool Entry::isRecycled() const
         return false;
     }
 
-    return m_group == db->metadata()->recycleBin() || m_group->isRecycled();
+    return m_group->isRecycled();
 }
 
 bool Entry::isAttributeReference(const QString& key) const

diff --git a/src/core/Group.cpp b/src/core/Group.cpp
@@ -223,18 +223,16 @@ Entry* Group::lastTopVisibleEntry() const
 bool Group::isRecycled() const
 {
     auto group = this;
-    if (!group->database()) {
+    if (!group->database() || !group->m_db->metadata()) {
         return false;
     }
 
     do {
-        if (group->m_parent && group->m_db->metadata()) {
-            if (group->m_parent == group->m_db->metadata()->recycleBin()) {
-                return true;
-            }
+        if (group == group->m_db->metadata()->recycleBin()) {
+            return true;
         }
         group = group->m_parent;
-    } while (group && group->m_parent && group->m_parent != group->m_db->rootGroup());
+    } while (group);
 
     return false;
 }

diff --git a/src/core/Group.h b/src/core/Group.h
@@ -230,9 +230,7 @@ private slots:
 
     bool m_updateTimeinfo;
 
-    friend void Database::setRootGroup(Group* group);
-    friend Entry::~Entry();
-    friend void Entry::setGroup(Group* group, bool trackPrevious);
+    friend Group* Database::setRootGroup(Group* group);
 };
 
 Q_DECLARE_OPERATORS_FOR_FLAGS(Group::CloneFlags)

diff --git a/src/fdosecrets/objects/Collection.cpp b/src/fdosecrets/objects/Collection.cpp
@@ -408,7 +408,7 @@ namespace FdoSecrets
 
         auto newUuid = FdoSecrets::settings()->exposedGroup(m_backend->database());
         auto newGroup = m_backend->database()->rootGroup()->findGroupByUuid(newUuid);
-        if (!newGroup || inRecycleBin(newGroup)) {
+        if (!newGroup || newGroup->isRecycled()) {
             // no exposed group, delete self
             removeFromDBus();
             return;
@@ -444,7 +444,7 @@ namespace FdoSecrets
         });
         // Another possibility is the group being moved to recycle bin.
         connect(m_exposedGroup.data(), &Group::modified, this, [this]() {
-            if (inRecycleBin(m_exposedGroup)) {
+            if (m_exposedGroup->isRecycled()) {
                 // reset the exposed group to none
                 FdoSecrets::settings()->setExposedGroup(m_backend->database().data(), {});
             }
@@ -490,7 +490,7 @@ namespace FdoSecrets
 
     void Collection::onEntryAdded(Entry* entry, bool emitSignal)
     {
-        if (inRecycleBin(entry)) {
+        if (entry->isRecycled()) {
             return;
         }
 
@@ -524,7 +524,7 @@ namespace FdoSecrets
 
     void Collection::connectGroupSignalRecursive(Group* group)
     {
-        if (inRecycleBin(group)) {
+        if (group->isRecycled()) {
             return;
         }
 
@@ -627,7 +627,7 @@ namespace FdoSecrets
     bool Collection::doDeleteEntry(Entry* entry)
     {
         // Confirm entry removal before moving forward
-        bool permanent = inRecycleBin(entry) || !m_backend->database()->metadata()->recycleBinEnabled();
+        bool permanent = entry->isRecycled() || !m_backend->database()->metadata()->recycleBinEnabled();
         if (FdoSecrets::settings()->confirmDeleteItem()
             && !GuiTools::confirmDeleteEntries(m_backend, {entry}, permanent)) {
             return false;
@@ -664,29 +664,6 @@ namespace FdoSecrets
         return group;
     }
 
-    bool Collection::inRecycleBin(Group* group) const
-    {
-        Q_ASSERT(m_backend);
-        Q_ASSERT(group);
-
-        if (!m_backend->database()->metadata()) {
-            return false;
-        }
-
-        auto recycleBin = m_backend->database()->metadata()->recycleBin();
-        if (!recycleBin) {
-            return false;
-        }
-
-        return group->uuid() == recycleBin->uuid() || group->isRecycled();
-    }
-
-    bool Collection::inRecycleBin(Entry* entry) const
-    {
-        Q_ASSERT(entry);
-        return inRecycleBin(entry->group());
-    }
-
     Item* Collection::doNewItem(const DBusClientPtr& client, QString itemPath)
     {
         Q_ASSERT(m_backend);

diff --git a/src/fdosecrets/objects/Collection.h b/src/fdosecrets/objects/Collection.h
@@ -107,11 +107,6 @@ namespace FdoSecrets
         DatabaseWidget* backend() const;
         QString backendFilePath() const;
         Service* service() const;
-        /**
-         * similar to Group::isRecycled, but we also return true when the group itself is the recycle bin
-         */
-        bool inRecycleBin(Group* group) const;
-        bool inRecycleBin(Entry* entry) const;
 
         static EntrySearcher::SearchTerm attributeToTerm(const QString& key, const QString& value);
 

diff --git a/src/format/KdbxXmlReader.cpp b/src/format/KdbxXmlReader.cpp
@@ -463,8 +463,7 @@ bool KdbxXmlReader::parseRoot()
 
             Group* rootGroup = parseGroup();
             if (rootGroup) {
-                Group* oldRoot = m_db->rootGroup();
-                m_db->setRootGroup(rootGroup);
+                auto oldRoot = m_db->setRootGroup(rootGroup);
                 delete oldRoot;
                 groupParsedSuccessfully = true;
             }

diff --git a/src/keeshare/ShareExport.cpp b/src/keeshare/ShareExport.cpp
@@ -92,8 +92,7 @@ namespace
         key->addKey(QSharedPointer<PasswordKey>::create(reference.password));
         targetDb->setKey(key);
 
-        auto* obsoleteRoot = targetDb->rootGroup();
-        targetDb->setRootGroup(targetRoot);
+        auto obsoleteRoot = targetDb->setRootGroup(targetRoot);
         delete obsoleteRoot;
 
         targetDb->metadata()->setName(sourceRoot->name());

diff --git a/src/keeshare/ShareObserver.cpp b/src/keeshare/ShareObserver.cpp
@@ -54,6 +54,7 @@ ShareObserver::ShareObserver(QSharedPointer<Database> db, QObject* parent)
 
 ShareObserver::~ShareObserver()
 {
+    m_db->disconnect(this);
 }
 
 void ShareObserver::deinitialize()

diff --git a/tests/TestAutoType.cpp b/tests/TestAutoType.cpp
@@ -62,8 +62,7 @@ void TestAutoType::init()
     m_db = QSharedPointer<Database>::create();
     m_dbList.clear();
     m_dbList.append(m_db);
-    m_group = new Group();
-    m_db->setRootGroup(m_group);
+    m_group = m_db->rootGroup();
 
     AutoTypeAssociations::Association association;
 

diff --git a/tests/TestCli.cpp b/tests/TestCli.cpp
@@ -1798,7 +1798,8 @@ void TestCli::testMergeWithKeys()
     entry->setPassword("secretsecretsecret");
     group->addEntry(entry);
 
-    sourceDatabase->setRootGroup(rootGroup);
+    auto oldGroup = sourceDatabase->setRootGroup(rootGroup);
+    delete oldGroup;
 
     auto* otherRootGroup = new Group();
     otherRootGroup->setName("root");
@@ -1814,7 +1815,8 @@ void TestCli::testMergeWithKeys()
     otherEntry->setPassword("secretsecretsecret 2");
     otherGroup->addEntry(otherEntry);
 
-    targetDatabase->setRootGroup(otherRootGroup);
+    oldGroup = targetDatabase->setRootGroup(otherRootGroup);
+    delete oldGroup;
 
     sourceDatabase->saveAs(sourceDatabaseFilename);
     targetDatabase->saveAs(targetDatabaseFilename);

diff --git a/tests/TestKeePass2Format.cpp b/tests/TestKeePass2Format.cpp
@@ -576,7 +576,9 @@ void TestKeePass2Format::testKdbxKeyChange()
     buffer.seek(0);
     QSharedPointer<Database> db(new Database());
     db->changeKdf(fastKdf(KeePass2::uuidToKdf(m_kdbxSourceDb->kdf()->uuid())));
-    db->setRootGroup(m_kdbxSourceDb->rootGroup()->clone(Entry::CloneNoFlags, Group::CloneIncludeEntries));
+    auto oldGroup =
+        db->setRootGroup(m_kdbxSourceDb->rootGroup()->clone(Entry::CloneNoFlags, Group::CloneIncludeEntries));
+    delete oldGroup;
 
     db->setKey(key1);
     writeKdbx(&buffer, db.data(), hasError, errorString);

diff --git a/tests/TestMerge.cpp b/tests/TestMerge.cpp
@@ -1544,9 +1544,9 @@ Database* TestMerge::createTestDatabase()
 
 Database* TestMerge::createTestDatabaseStructureClone(Database* source, int entryFlags, int groupFlags)
 {
-    Database* db = new Database();
-    // the old root group is deleted by QObject::parent relationship
-    db->setRootGroup(source->rootGroup()->clone(static_cast<Entry::CloneFlag>(entryFlags),
-                                                static_cast<Group::CloneFlag>(groupFlags)));
+    auto db = new Database();
+    auto oldGroup = db->setRootGroup(source->rootGroup()->clone(static_cast<Entry::CloneFlag>(entryFlags),
+                                                                static_cast<Group::CloneFlag>(groupFlags)));
+    delete oldGroup;
     return db;
 }