From 48ea2c10abe48b571f3c835b5937c0b74905291c Mon Sep 17 00:00:00 2001 From: varjolintu Date: Fri, 1 May 2020 11:07:14 +0300 Subject: [PATCH] Fix merging browser keys * Introduce protected custom data function to prevent loss during merge operations --- src/browser/BrowserService.cpp | 19 +++++++++---------- src/browser/BrowserService.h | 2 -- src/core/CustomData.cpp | 7 +++++++ src/core/CustomData.h | 3 +++ src/core/Merger.cpp | 4 +++- .../DatabaseSettingsWidgetBrowser.cpp | 14 +++++++------- 6 files changed, 29 insertions(+), 20 deletions(-) diff --git a/src/browser/BrowserService.cpp b/src/browser/BrowserService.cpp index b83af627a4..91ad77432c 100644 --- a/src/browser/BrowserService.cpp +++ b/src/browser/BrowserService.cpp @@ -46,11 +46,9 @@ const QString BrowserService::KEEPASSXCBROWSER_NAME = QStringLiteral("KeePassXC-Browser Settings"); const QString BrowserService::KEEPASSXCBROWSER_OLD_NAME = QStringLiteral("keepassxc-browser Settings"); -const QString BrowserService::ASSOCIATE_KEY_PREFIX = QStringLiteral("KPXC_BROWSER_"); static const QString KEEPASSXCBROWSER_GROUP_NAME = QStringLiteral("KeePassXC-Browser Passwords"); static int KEEPASSXCBROWSER_DEFAULT_ICON = 1; // These are for the settings and password conversion -const QString BrowserService::LEGACY_ASSOCIATE_KEY_PREFIX = QStringLiteral("Public Key: "); static const QString KEEPASSHTTP_NAME = QStringLiteral("KeePassHttp Settings"); static const QString KEEPASSHTTP_GROUP_NAME = QStringLiteral("KeePassHttp Passwords"); // Extra entry related options saved in custom data @@ -318,7 +316,7 @@ QString BrowserService::storeKey(const QString& key) return {}; } - contains = db->metadata()->customData()->contains(ASSOCIATE_KEY_PREFIX + id); + contains = db->metadata()->customData()->contains(CustomData::BrowserKeyPrefix + id); if (contains) { dialogResult = MessageBox::warning(nullptr, tr("KeePassXC: Overwrite existing key?"), @@ -331,7 +329,7 @@ QString BrowserService::storeKey(const QString& key) } while (contains && dialogResult == MessageBox::Cancel); hideWindow(); - db->metadata()->customData()->set(ASSOCIATE_KEY_PREFIX + id, key); + db->metadata()->customData()->set(CustomData::BrowserKeyPrefix + id, key); db->metadata()->customData()->set(QString("%1_%2").arg(CustomData::Created, id), Clock::currentDateTime().toString(Qt::SystemLocaleShortDate)); return id; @@ -344,7 +342,7 @@ QString BrowserService::getKey(const QString& id) return {}; } - return db->metadata()->customData()->value(ASSOCIATE_KEY_PREFIX + id); + return db->metadata()->customData()->value(CustomData::BrowserKeyPrefix + id); } QJsonArray BrowserService::findMatchingEntries(const QString& dbid, @@ -590,7 +588,7 @@ QList BrowserService::searchEntries(const QString& url, const QString& s // Check if database is connected with KeePassXC-Browser auto databaseConnected = [&](const QSharedPointer& db) { for (const StringPair& keyPair : keyList) { - QString key = db->metadata()->customData()->value(ASSOCIATE_KEY_PREFIX + keyPair.first); + QString key = db->metadata()->customData()->value(CustomData::BrowserKeyPrefix + keyPair.first); if (!key.isEmpty() && keyPair.second == key) { return true; } @@ -1108,13 +1106,14 @@ int BrowserService::moveKeysToCustomData(Entry* entry, QSharedPointer { int keyCounter = 0; for (const auto& key : entry->attributes()->keys()) { - if (key.contains(LEGACY_ASSOCIATE_KEY_PREFIX)) { + if (key.contains(CustomData::BrowserLegacyKeyPrefix)) { QString publicKey = key; - publicKey.remove(LEGACY_ASSOCIATE_KEY_PREFIX); + publicKey.remove(CustomData::BrowserLegacyKeyPrefix); // Add key to database custom data - if (db && !db->metadata()->customData()->contains(ASSOCIATE_KEY_PREFIX + publicKey)) { - db->metadata()->customData()->set(ASSOCIATE_KEY_PREFIX + publicKey, entry->attributes()->value(key)); + if (db && !db->metadata()->customData()->contains(CustomData::BrowserKeyPrefix + publicKey)) { + db->metadata()->customData()->set(CustomData::BrowserKeyPrefix + publicKey, + entry->attributes()->value(key)); ++keyCounter; } } diff --git a/src/browser/BrowserService.h b/src/browser/BrowserService.h index 6de5e49bfe..2d084e74c8 100644 --- a/src/browser/BrowserService.h +++ b/src/browser/BrowserService.h @@ -87,8 +87,6 @@ class BrowserService : public QObject static const QString KEEPASSXCBROWSER_NAME; static const QString KEEPASSXCBROWSER_OLD_NAME; - static const QString ASSOCIATE_KEY_PREFIX; - static const QString LEGACY_ASSOCIATE_KEY_PREFIX; static const QString OPTION_SKIP_AUTO_SUBMIT; static const QString OPTION_HIDE_ENTRY; static const QString OPTION_ONLY_HTTP_AUTH; diff --git a/src/core/CustomData.cpp b/src/core/CustomData.cpp index 95aee805d1..b421ec3a06 100644 --- a/src/core/CustomData.cpp +++ b/src/core/CustomData.cpp @@ -22,6 +22,8 @@ const QString CustomData::LastModified = QStringLiteral("_LAST_MODIFIED"); const QString CustomData::Created = QStringLiteral("_CREATED"); +const QString CustomData::BrowserKeyPrefix = QStringLiteral("KPXC_BROWSER_"); +const QString CustomData::BrowserLegacyKeyPrefix = QStringLiteral("Public Key: "); CustomData::CustomData(QObject* parent) : QObject(parent) @@ -128,6 +130,11 @@ QDateTime CustomData::getLastModified() const return {}; } +bool CustomData::isProtectedCustomData(const QString& key) const +{ + return key.startsWith(CustomData::BrowserKeyPrefix) || key.startsWith(CustomData::Created); +} + bool CustomData::operator==(const CustomData& other) const { return (m_data == other.m_data); diff --git a/src/core/CustomData.h b/src/core/CustomData.h index 212765f769..93b78c46a6 100644 --- a/src/core/CustomData.h +++ b/src/core/CustomData.h @@ -43,11 +43,14 @@ class CustomData : public QObject int dataSize() const; void copyDataFrom(const CustomData* other); QDateTime getLastModified() const; + bool isProtectedCustomData(const QString& key) const; bool operator==(const CustomData& other) const; bool operator!=(const CustomData& other) const; static const QString LastModified; static const QString Created; + static const QString BrowserKeyPrefix; + static const QString BrowserLegacyKeyPrefix; signals: void customDataModified(); diff --git a/src/core/Merger.cpp b/src/core/Merger.cpp index 4cce997bfd..2becd8a689 100644 --- a/src/core/Merger.cpp +++ b/src/core/Merger.cpp @@ -632,7 +632,9 @@ Merger::ChangeList Merger::mergeMetadata(const MergeContext& context) // Check missing keys from source. Remove those from target for (const auto& key : targetCustomDataKeys) { - if (!sourceMetadata->customData()->contains(key)) { + // Do not remove protected custom data + if (!sourceMetadata->customData()->contains(key) + && !sourceMetadata->customData()->isProtectedCustomData(key)) { auto value = targetMetadata->customData()->value(key); targetMetadata->customData()->remove(key); changes << tr("Removed custom data %1 [%2]").arg(key, value); diff --git a/src/gui/dbsettings/DatabaseSettingsWidgetBrowser.cpp b/src/gui/dbsettings/DatabaseSettingsWidgetBrowser.cpp index 63659c22ba..746e6a66ec 100644 --- a/src/gui/dbsettings/DatabaseSettingsWidgetBrowser.cpp +++ b/src/gui/dbsettings/DatabaseSettingsWidgetBrowser.cpp @@ -107,7 +107,7 @@ void DatabaseSettingsWidgetBrowser::removeSelectedKey() if (itemSelectionModel) { for (const QModelIndex& index : itemSelectionModel->selectedRows(0)) { QString key = index.data().toString(); - key.insert(0, BrowserService::ASSOCIATE_KEY_PREFIX); + key.insert(0, CustomData::BrowserKeyPrefix); customData()->remove(key); } updateModel(); @@ -125,9 +125,9 @@ void DatabaseSettingsWidgetBrowser::updateModel() m_customDataModel->setHorizontalHeaderLabels({tr("Key"), tr("Value"), tr("Created")}); for (const QString& key : customData()->keys()) { - if (key.startsWith(BrowserService::ASSOCIATE_KEY_PREFIX)) { + if (key.startsWith(CustomData::BrowserKeyPrefix)) { QString strippedKey = key; - strippedKey.remove(BrowserService::ASSOCIATE_KEY_PREFIX); + strippedKey.remove(CustomData::BrowserKeyPrefix); auto created = customData()->value(QString("%1_%2").arg(CustomData::Created, strippedKey)); auto createdItem = new QStandardItem(created); createdItem->setEditable(false); @@ -174,7 +174,7 @@ void DatabaseSettingsWidgetBrowser::removeSharedEncryptionKeys() QStringList keysToRemove; for (const QString& key : m_db->metadata()->customData()->keys()) { - if (key.startsWith(BrowserService::ASSOCIATE_KEY_PREFIX)) { + if (key.startsWith(CustomData::BrowserKeyPrefix)) { keysToRemove << key; } } @@ -299,16 +299,16 @@ void DatabaseSettingsWidgetBrowser::editFinished(QStandardItem* item) // The key is edited if (item->column() == 0) { // Get the old key/value pair, remove it and replace it - m_valueInEdit.insert(0, BrowserService::ASSOCIATE_KEY_PREFIX); + m_valueInEdit.insert(0, CustomData::BrowserKeyPrefix); auto tempValue = customData()->value(m_valueInEdit); - newValue.insert(0, BrowserService::ASSOCIATE_KEY_PREFIX); + newValue.insert(0, CustomData::BrowserKeyPrefix); m_db->metadata()->customData()->remove(m_valueInEdit); m_db->metadata()->customData()->set(newValue, tempValue); } else { // Replace just the value for (const QString& key : m_db->metadata()->customData()->keys()) { - if (key.startsWith(BrowserService::ASSOCIATE_KEY_PREFIX)) { + if (key.startsWith(CustomData::BrowserKeyPrefix)) { if (m_valueInEdit == m_db->metadata()->customData()->value(key)) { m_db->metadata()->customData()->set(key, newValue); break;