diff --git a/src/core/Database.cpp b/src/core/Database.cpp index aa36dad12..562159aad 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); @@ -481,9 +481,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(); @@ -559,14 +558,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); @@ -574,8 +571,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 d4a0a7bd5..31a839734 100644 --- a/src/core/Database.h +++ b/src/core/Database.h @@ -112,7 +112,7 @@ public: 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 d384654a6..b57fd78f1 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 43fda39a4..6991af55d 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 e934e5a65..85a4317a3 100644 --- a/src/core/Group.h +++ b/src/core/Group.h @@ -228,9 +228,7 @@ private: 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 be452d429..d16d9f552 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 06e8467e5..c8a49ef35 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 eb7fa4158..b51d54f36 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 18a971418..1a64ccde0 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 ac1c44baa..5a0da292d 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 445735f9f..eea1f0532 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 fa1571637..71246d9bf 100644 --- a/tests/TestCli.cpp +++ b/tests/TestCli.cpp @@ -1799,7 +1799,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"); @@ -1815,7 +1816,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 dc928fa14..a1c7b20d4 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 fb9b32b43..d2fd5a000 100644 --- a/tests/TestMerge.cpp +++ b/tests/TestMerge.cpp @@ -1456,8 +1456,8 @@ Database* TestMerge::createTestDatabase() Database* TestMerge::createTestDatabaseStructureClone(Database* source, int entryFlags, int groupFlags) { auto 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 oldGroup = db->setRootGroup(source->rootGroup()->clone(static_cast(entryFlags), + static_cast(groupFlags))); + delete oldGroup; return db; }