From dae2f7a90fa09cb00da271af807f42b74135d570 Mon Sep 17 00:00:00 2001 From: Jonathan White Date: Sat, 9 Mar 2024 11:52:29 -0500 Subject: [PATCH] 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 --- src/core/Database.cpp | 32 +++++++++++++++----------- src/core/Database.h | 2 +- src/core/Entry.cpp | 2 +- src/core/Group.cpp | 10 ++++---- src/core/Group.h | 4 +--- src/fdosecrets/objects/Collection.cpp | 33 ++++----------------------- src/fdosecrets/objects/Collection.h | 5 ---- src/format/KdbxXmlReader.cpp | 3 +-- src/keeshare/ShareExport.cpp | 3 +-- src/keeshare/ShareObserver.cpp | 1 + tests/TestAutoType.cpp | 3 +-- tests/TestCli.cpp | 6 +++-- tests/TestKeePass2Format.cpp | 4 +++- tests/TestMerge.cpp | 8 +++---- 14 files changed, 46 insertions(+), 70 deletions(-) diff --git a/src/core/Database.cpp b/src/core/Database.cpp index 8341521932..20587339bb 100644 --- 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,14 +556,12 @@ 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); @@ -572,8 +569,17 @@ void Database::setRootGroup(Group* group) 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 index 6d8e0403bf..45e5140fda 100644 --- 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 index e237ae53d5..db1542c6c4 100644 --- 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 index 687409e4e4..fb9d429214 100644 --- 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 index e649cda49d..8d5a78d491 100644 --- 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 index be452d4299..d16d9f552d 100644 --- 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 index 06e8467e53..c8a49ef355 100644 --- 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 index e66e12174c..a610cddb18 100644 --- 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 index 670a2911ea..49fa128b8d 100644 --- a/src/keeshare/ShareExport.cpp +++ b/src/keeshare/ShareExport.cpp @@ -92,8 +92,7 @@ namespace key->addKey(QSharedPointer::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 index ac1c44baa9..5a0da292d6 100644 --- a/src/keeshare/ShareObserver.cpp +++ b/src/keeshare/ShareObserver.cpp @@ -54,6 +54,7 @@ ShareObserver::ShareObserver(QSharedPointer db, QObject* parent) ShareObserver::~ShareObserver() { + m_db->disconnect(this); } void ShareObserver::deinitialize() diff --git a/tests/TestAutoType.cpp b/tests/TestAutoType.cpp index 445735f9fc..eea1f05325 100644 --- a/tests/TestAutoType.cpp +++ b/tests/TestAutoType.cpp @@ -62,8 +62,7 @@ void TestAutoType::init() m_db = QSharedPointer::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 index f941c92893..1445a8def6 100644 --- 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 index dc928fa143..a1c7b20d46 100644 --- a/tests/TestKeePass2Format.cpp +++ b/tests/TestKeePass2Format.cpp @@ -576,7 +576,9 @@ void TestKeePass2Format::testKdbxKeyChange() buffer.seek(0); QSharedPointer 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 index 014f39f414..3f64c908a9 100644 --- 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(entryFlags), - static_cast(groupFlags))); + auto db = new Database(); + auto oldGroup = db->setRootGroup(source->rootGroup()->clone(static_cast(entryFlags), + static_cast(groupFlags))); + delete oldGroup; return db; }