From bce8c84c26edb1de7fbd31527a3906639aee096f Mon Sep 17 00:00:00 2001 From: Patrick Klein <42714034+libklein@users.noreply.github.com> Date: Sat, 29 Jan 2022 03:26:53 +0100 Subject: [PATCH] Remove obsolete read only state from database. (#7324) --- share/translations/keepassxc_en.ts | 17 ----------- src/cli/Merge.cpp | 2 +- src/cli/Utils.cpp | 2 +- src/core/Database.cpp | 47 ++---------------------------- src/core/Database.h | 21 ++++++------- src/gui/DatabaseOpenWidget.cpp | 2 +- src/gui/DatabaseTabWidget.cpp | 20 +------------ src/gui/DatabaseTabWidget.h | 1 - src/gui/DatabaseWidget.cpp | 9 ++---- tests/TestDatabase.cpp | 15 +++------- tests/TestKdbx3.cpp | 8 ++--- tests/TestKeys.cpp | 2 +- tests/gui/TestGui.cpp | 2 +- 13 files changed, 28 insertions(+), 120 deletions(-) diff --git a/share/translations/keepassxc_en.ts b/share/translations/keepassxc_en.ts index 198be235..945527b2 100644 --- a/share/translations/keepassxc_en.ts +++ b/share/translations/keepassxc_en.ts @@ -1342,10 +1342,6 @@ Do you want to delete the entry? Error while reading the database: %1 Error while reading the database: %1 - - File cannot be written as it is opened in read-only mode. - File cannot be written as it is opened in read-only mode. - %1 Backup database located at %2 @@ -1355,10 +1351,6 @@ Backup database located at %2 Could not save, database does not point to a valid file. - - Could not save, database file is read-only. - - Database file has unmerged changes. @@ -2208,11 +2200,6 @@ This is definitely a bug, please report it to the developers. Database tab name modifier %1 [Locked] - - %1 [Read-only] - Database tab name modifier - %1 [Read-only] - Failed to open %1. It either does not exist or is not accessible. @@ -2370,10 +2357,6 @@ Disable safe saves and try again? Writing the database failed: %1 Writing the database failed: %1 - - This database is opened in read-only mode. Autosave is disabled. - - Save database backup diff --git a/src/cli/Merge.cpp b/src/cli/Merge.cpp index ade472af..410892c9 100644 --- a/src/cli/Merge.cpp +++ b/src/cli/Merge.cpp @@ -80,7 +80,7 @@ int Merge::executeWithDatabase(QSharedPointer database, QSharedPointer } else { db2 = QSharedPointer::create(); QString errorMessage; - if (!db2->open(fromDatabasePath, database->key(), &errorMessage, false)) { + if (!db2->open(fromDatabasePath, database->key(), &errorMessage)) { err << QObject::tr("Error reading merge file:\n%1").arg(errorMessage); return EXIT_FAILURE; } diff --git a/src/cli/Utils.cpp b/src/cli/Utils.cpp index 69c84372..f90f38b0 100644 --- a/src/cli/Utils.cpp +++ b/src/cli/Utils.cpp @@ -182,7 +182,7 @@ namespace Utils auto db = QSharedPointer::create(); QString error; - if (db->open(databaseFilename, compositeKey, &error, false)) { + if (db->open(databaseFilename, compositeKey, &error)) { return db; } else { err << error << endl; diff --git a/src/core/Database.cpp b/src/core/Database.cpp index 0f85546f..217c9e18 100644 --- a/src/core/Database.cpp +++ b/src/core/Database.cpp @@ -96,17 +96,16 @@ QUuid Database::uuid() const * read-write mode and fall back to read-only if that is not possible. * * @param key composite key for unlocking the database - * @param readOnly open in read-only mode * @param error error message in case of failure * @return true on success */ -bool Database::open(QSharedPointer key, QString* error, bool readOnly) +bool Database::open(QSharedPointer key, QString* error) { Q_ASSERT(!m_data.filePath.isEmpty()); if (m_data.filePath.isEmpty()) { return false; } - return open(m_data.filePath, std::move(key), error, readOnly); + return open(m_data.filePath, std::move(key), error); } /** @@ -116,11 +115,10 @@ bool Database::open(QSharedPointer key, QString* error, bool * * @param filePath path to the file * @param key composite key for unlocking the database - * @param readOnly open in read-only mode * @param error error message in case of failure * @return true on success */ -bool Database::open(const QString& filePath, QSharedPointer key, QString* error, bool readOnly) +bool Database::open(const QString& filePath, QSharedPointer key, QString* error) { QFile dbFile(filePath); if (!dbFile.exists()) { @@ -154,7 +152,6 @@ bool Database::open(const QString& filePath, QSharedPointer return false; } - setReadOnly(readOnly); setFilePath(filePath); dbFile.close(); @@ -260,15 +257,6 @@ bool Database::saveAs(const QString& filePath, SaveAction action, const QString& QMutexLocker locker(&m_saveMutex); if (filePath == m_data.filePath) { - // Disallow saving to the same file if read-only - if (m_data.isReadOnly) { - Q_ASSERT_X(false, "Database::saveAs", "Could not save, database file is read-only."); - if (error) { - *error = tr("Could not save, database file is read-only."); - } - return false; - } - // Fail-safe check to make sure we don't overwrite underlying file changes // that have not yet triggered a file reload/merge operation. if (!m_fileWatcher->hasSameFileChecksum()) { @@ -280,7 +268,6 @@ bool Database::saveAs(const QString& filePath, SaveAction action, const QString& } // Clear read-only flag - setReadOnly(false); m_fileWatcher->stop(); QFileInfo fileInfo(filePath); @@ -402,14 +389,6 @@ bool Database::performSave(const QString& filePath, SaveAction action, const QSt bool Database::writeDatabase(QIODevice* device, QString* error) { - Q_ASSERT(!m_data.isReadOnly); - if (m_data.isReadOnly) { - if (error) { - *error = tr("File cannot be written as it is opened in read-only mode."); - } - return false; - } - PasswordKey oldTransformedKey; if (m_data.key->isEmpty()) { oldTransformedKey.setHash(m_data.transformedDatabaseKey->rawKey()); @@ -556,16 +535,6 @@ bool Database::restoreDatabase(const QString& filePath, const QString& fromBacku return false; } -bool Database::isReadOnly() const -{ - return m_data.isReadOnly; -} - -void Database::setReadOnly(bool readOnly) -{ - m_data.isReadOnly = readOnly; -} - /** * Returns true if the database key exists, has subkeys, and the * root group exists @@ -811,7 +780,6 @@ bool Database::setKey(const QSharedPointer& key, bool updateTransformSalt, bool transformKey) { - Q_ASSERT(!m_data.isReadOnly); m_keyError.clear(); if (!key) { @@ -870,14 +838,11 @@ const QVariantMap& Database::publicCustomData() const void Database::setPublicCustomData(const QVariantMap& customData) { - Q_ASSERT(!m_data.isReadOnly); m_data.publicCustomData = customData; } void Database::createRecycleBin() { - Q_ASSERT(!m_data.isReadOnly); - auto recycleBin = new Group(); recycleBin->setUuid(QUuid::createUuid()); recycleBin->setParent(rootGroup()); @@ -891,7 +856,6 @@ void Database::createRecycleBin() void Database::recycleEntry(Entry* entry) { - Q_ASSERT(!m_data.isReadOnly); if (m_metadata->recycleBinEnabled()) { if (!m_metadata->recycleBin()) { createRecycleBin(); @@ -904,7 +868,6 @@ void Database::recycleEntry(Entry* entry) void Database::recycleGroup(Group* group) { - Q_ASSERT(!m_data.isReadOnly); if (m_metadata->recycleBinEnabled()) { if (!m_metadata->recycleBin()) { createRecycleBin(); @@ -917,7 +880,6 @@ void Database::recycleGroup(Group* group) void Database::emptyRecycleBin() { - Q_ASSERT(!m_data.isReadOnly); if (m_metadata->recycleBinEnabled() && m_metadata->recycleBin()) { // destroying direct entries of the recycle bin QList subEntries = m_metadata->recycleBin()->entries(); @@ -988,15 +950,12 @@ QSharedPointer Database::kdf() const void Database::setKdf(QSharedPointer kdf) { - Q_ASSERT(!m_data.isReadOnly); m_data.kdf = std::move(kdf); setFormatVersion(KeePass2Writer::kdbxVersionRequired(this, true, m_data.kdf.isNull())); } bool Database::changeKdf(const QSharedPointer& kdf) { - Q_ASSERT(!m_data.isReadOnly); - kdf->randomizeSeed(); QByteArray transformedDatabaseKey; if (!m_data.key) { diff --git a/src/core/Database.h b/src/core/Database.h index 77abf430..bad0b256 100644 --- a/src/core/Database.h +++ b/src/core/Database.h @@ -74,11 +74,15 @@ public: explicit Database(const QString& filePath); ~Database() override; - bool open(QSharedPointer key, QString* error = nullptr, bool readOnly = false); - bool open(const QString& filePath, - QSharedPointer key, - QString* error = nullptr, - bool readOnly = false); +private: + bool writeDatabase(QIODevice* device, QString* error = nullptr); + bool backupDatabase(const QString& filePath, const QString& destinationFilePath); + bool restoreDatabase(const QString& filePath, const QString& fromBackupFilePath); + bool performSave(const QString& filePath, SaveAction flags, const QString& backupFilePath, QString* error); + +public: + bool open(QSharedPointer key, QString* error = nullptr); + bool open(const QString& filePath, QSharedPointer key, QString* error = nullptr); bool save(SaveAction action = Atomic, const QString& backupFilePath = QString(), QString* error = nullptr); bool saveAs(const QString& filePath, SaveAction action = Atomic, @@ -96,8 +100,6 @@ public: bool isInitialized() const; bool isModified() const; bool hasNonDataChanges() const; - bool isReadOnly() const; - void setReadOnly(bool readOnly); bool isSaving(); QUuid uuid() const; @@ -175,7 +177,6 @@ private: { quint32 formatVersion = 0; QString filePath; - bool isReadOnly = false; QUuid cipher = KeePass2::CIPHER_AES256; CompressionAlgorithm compressionAlgorithm = CompressionGZip; @@ -213,10 +214,6 @@ private: void createRecycleBin(); - bool writeDatabase(QIODevice* device, QString* error = nullptr); - bool backupDatabase(const QString& filePath, const QString& destinationFilePath); - bool restoreDatabase(const QString& filePath, const QString& fromBackupFilePath); - bool performSave(const QString& filePath, SaveAction flags, const QString& backupFilePath, QString* error); void startModifiedTimer(); void stopModifiedTimer(); diff --git a/src/gui/DatabaseOpenWidget.cpp b/src/gui/DatabaseOpenWidget.cpp index f0d494b2..1d51b5eb 100644 --- a/src/gui/DatabaseOpenWidget.cpp +++ b/src/gui/DatabaseOpenWidget.cpp @@ -202,7 +202,7 @@ void DatabaseOpenWidget::openDatabase() QApplication::setOverrideCursor(QCursor(Qt::WaitCursor)); m_ui->passwordFormFrame->setEnabled(false); QCoreApplication::processEvents(); - bool ok = m_db->open(m_filename, databaseKey, &error, false); + bool ok = m_db->open(m_filename, databaseKey, &error); QApplication::restoreOverrideCursor(); m_ui->passwordFormFrame->setEnabled(true); diff --git a/src/gui/DatabaseTabWidget.cpp b/src/gui/DatabaseTabWidget.cpp index 74ecc0a6..02affe6f 100644 --- a/src/gui/DatabaseTabWidget.cpp +++ b/src/gui/DatabaseTabWidget.cpp @@ -513,20 +513,6 @@ void DatabaseTabWidget::showDatabaseSettings() currentDatabaseWidget()->switchToDatabaseSettings(); } -bool DatabaseTabWidget::isReadOnly(int index) const -{ - if (count() == 0) { - return false; - } - - if (index == -1) { - index = currentIndex(); - } - - auto db = databaseWidgetFromIndex(index)->database(); - return db && db->isReadOnly(); -} - bool DatabaseTabWidget::isModified(int index) const { if (count() == 0) { @@ -543,7 +529,7 @@ bool DatabaseTabWidget::isModified(int index) const bool DatabaseTabWidget::canSave(int index) const { - return !isReadOnly(index) && isModified(index); + return isModified(index); } bool DatabaseTabWidget::hasLockableDatabases() const @@ -601,10 +587,6 @@ QString DatabaseTabWidget::tabName(int index) tabName = tr("%1 [Locked]", "Database tab name modifier").arg(tabName); } - if (db->isReadOnly()) { - tabName = tr("%1 [Read-only]", "Database tab name modifier").arg(tabName); - } - if (db->isModified()) { tabName.append("*"); } diff --git a/src/gui/DatabaseTabWidget.h b/src/gui/DatabaseTabWidget.h index ffcd9748..8432116e 100644 --- a/src/gui/DatabaseTabWidget.h +++ b/src/gui/DatabaseTabWidget.h @@ -41,7 +41,6 @@ public: DatabaseWidget* currentDatabaseWidget(); DatabaseWidget* databaseWidgetFromIndex(int index) const; - bool isReadOnly(int index = -1) const; bool canSave(int index = -1) const; bool isModified(int index = -1) const; bool hasLockableDatabases() const; diff --git a/src/gui/DatabaseWidget.cpp b/src/gui/DatabaseWidget.cpp index 5fe758ca..b97aaf97 100644 --- a/src/gui/DatabaseWidget.cpp +++ b/src/gui/DatabaseWidget.cpp @@ -1201,10 +1201,6 @@ void DatabaseWidget::unlockDatabase(bool accepted) db = m_databaseOpenWidget->database(); } replaceDatabase(db); - if (db->isReadOnly()) { - showMessage( - tr("This database is opened in read-only mode. Autosave is disabled."), MessageWidget::Warning, false, -1); - } restoreGroupEntryFocus(m_groupBeforeLock, m_entryBeforeLock); m_groupBeforeLock = QUuid(); @@ -1466,7 +1462,7 @@ void DatabaseWidget::onGroupChanged() void DatabaseWidget::onDatabaseModified() { - if (!m_blockAutoSave && config()->get(Config::AutoSaveAfterEveryChange).toBool() && !m_db->isReadOnly()) { + if (!m_blockAutoSave && config()->get(Config::AutoSaveAfterEveryChange).toBool()) { save(); } else { // Only block once, then reset @@ -1900,7 +1896,7 @@ bool DatabaseWidget::save() } // Read-only and new databases ask for filename - if (m_db->isReadOnly() || m_db->filePath().isEmpty()) { + if (m_db->filePath().isEmpty()) { return saveAs(); } @@ -2056,7 +2052,6 @@ bool DatabaseWidget::saveBackup() if (!newFilePath.isEmpty()) { // Ensure we don't recurse back into this function - m_db->setReadOnly(false); m_db->setFilePath(newFilePath); m_saveAttempts = 0; diff --git a/tests/TestDatabase.cpp b/tests/TestDatabase.cpp index 0bcf2c6f..3b586427 100644 --- a/tests/TestDatabase.cpp +++ b/tests/TestDatabase.cpp @@ -144,11 +144,7 @@ void TestDatabase::testEmptyRecycleBinOnDisabled() auto key = QSharedPointer::create(); key->addKey(QSharedPointer::create("123")); auto db = QSharedPointer::create(); - QVERIFY(db->open(filename, key, nullptr, false)); - - // Explicitly mark DB as read-write in case it was opened from a read-only drive. - // Prevents assertion failures on CI systems when the data dir is not writable - db->setReadOnly(false); + QVERIFY(db->open(filename, key, nullptr)); QSignalSpy spyModified(db.data(), SIGNAL(modified())); @@ -163,8 +159,7 @@ void TestDatabase::testEmptyRecycleBinOnNotCreated() auto key = QSharedPointer::create(); key->addKey(QSharedPointer::create("123")); auto db = QSharedPointer::create(); - QVERIFY(db->open(filename, key, nullptr, false)); - db->setReadOnly(false); + QVERIFY(db->open(filename, key, nullptr)); QSignalSpy spyModified(db.data(), SIGNAL(modified())); @@ -179,8 +174,7 @@ void TestDatabase::testEmptyRecycleBinOnEmpty() auto key = QSharedPointer::create(); key->addKey(QSharedPointer::create("123")); auto db = QSharedPointer::create(); - QVERIFY(db->open(filename, key, nullptr, false)); - db->setReadOnly(false); + QVERIFY(db->open(filename, key, nullptr)); QSignalSpy spyModified(db.data(), SIGNAL(modified())); @@ -195,8 +189,7 @@ void TestDatabase::testEmptyRecycleBinWithHierarchicalData() auto key = QSharedPointer::create(); key->addKey(QSharedPointer::create("123")); auto db = QSharedPointer::create(); - QVERIFY(db->open(filename, key, nullptr, false)); - db->setReadOnly(false); + QVERIFY(db->open(filename, key, nullptr)); QFile originalFile(filename); qint64 initialSize = originalFile.size(); diff --git a/tests/TestKdbx3.cpp b/tests/TestKdbx3.cpp index bab8ab8d..bcc3df77 100644 --- a/tests/TestKdbx3.cpp +++ b/tests/TestKdbx3.cpp @@ -112,7 +112,7 @@ void TestKdbx3::testNonAscii() key->addKey(QSharedPointer::create(QString::fromUtf8("\xce\x94\xc3\xb6\xd8\xb6"))); KeePass2Reader reader; auto db = QSharedPointer::create(); - QVERIFY(db->open(filename, key, nullptr, false)); + QVERIFY(db->open(filename, key, nullptr)); QVERIFY(db.data()); QVERIFY(!reader.hasError()); QCOMPARE(db->metadata()->name(), QString("NonAsciiTest")); @@ -126,7 +126,7 @@ void TestKdbx3::testCompressed() key->addKey(QSharedPointer::create("")); KeePass2Reader reader; auto db = QSharedPointer::create(); - QVERIFY(db->open(filename, key, nullptr, false)); + QVERIFY(db->open(filename, key, nullptr)); QVERIFY(db.data()); QVERIFY(!reader.hasError()); QCOMPARE(db->metadata()->name(), QString("Compressed")); @@ -140,7 +140,7 @@ void TestKdbx3::testProtectedStrings() key->addKey(QSharedPointer::create("masterpw")); KeePass2Reader reader; auto db = QSharedPointer::create(); - QVERIFY(db->open(filename, key, nullptr, false)); + QVERIFY(db->open(filename, key, nullptr)); QVERIFY(db.data()); QVERIFY(!reader.hasError()); QCOMPARE(db->metadata()->name(), QString("Protected Strings Test")); @@ -167,5 +167,5 @@ void TestKdbx3::testBrokenHeaderHash() auto key = QSharedPointer::create(); key->addKey(QSharedPointer::create("")); auto db = QSharedPointer::create(); - QVERIFY(!db->open(filename, key, nullptr, false)); + QVERIFY(!db->open(filename, key, nullptr)); } diff --git a/tests/TestKeys.cpp b/tests/TestKeys.cpp index 2530db66..43a50adb 100644 --- a/tests/TestKeys.cpp +++ b/tests/TestKeys.cpp @@ -104,7 +104,7 @@ void TestKeys::testFileKey() compositeKey->addKey(fileKey); auto db = QSharedPointer::create(); - QVERIFY(db->open(dbFilename, compositeKey, nullptr, false)); + QVERIFY(db->open(dbFilename, compositeKey, nullptr)); QVERIFY(!reader.hasError()); QCOMPARE(db->metadata()->name(), QString("%1 Database").arg(name)); } diff --git a/tests/gui/TestGui.cpp b/tests/gui/TestGui.cpp index 06b3d301..63f0d3e0 100644 --- a/tests/gui/TestGui.cpp +++ b/tests/gui/TestGui.cpp @@ -1782,7 +1782,7 @@ void TestGui::checkDatabase(const QString& filePath, const QString& expectedDbNa auto key = QSharedPointer::create(); key->addKey(QSharedPointer::create("a")); auto dbSaved = QSharedPointer::create(); - QVERIFY(dbSaved->open(filePath, key, nullptr, false)); + QVERIFY(dbSaved->open(filePath, key, nullptr)); QCOMPARE(dbSaved->metadata()->name(), expectedDbName); }