Skip to content

Commit

Permalink
Fix merging browser keys
Browse files Browse the repository at this point in the history 
* Introduce protected custom data function to prevent loss during merge operations
  • Loading branch information
@droidmonkey
varjolintu authored and droidmonkey committed May 16, 2020
1 parent 45848c3 commit 48ea2c1
Show file tree
Hide file tree
Showing 6 changed files with 29 additions and 20 deletions.
19 changes: 9 additions & 10 deletions src/browser/BrowserService.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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?"),
Expand All @@ -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;
Expand All @@ -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,
Expand Down Expand Up @@ -590,7 +588,7 @@ QList<Entry*> BrowserService::searchEntries(const QString& url, const QString& s
// Check if database is connected with KeePassXC-Browser
auto databaseConnected = [&](const QSharedPointer<Database>& 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;
}
Expand Down Expand Up @@ -1108,13 +1106,14 @@ int BrowserService::moveKeysToCustomData(Entry* entry, QSharedPointer<Database>
{
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;
}
}
Expand Down
2 changes: 0 additions & 2 deletions src/browser/BrowserService.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
7 changes: 7 additions & 0 deletions src/core/CustomData.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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);
Expand Down
3 changes: 3 additions & 0 deletions src/core/CustomData.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
4 changes: 3 additions & 1 deletion src/core/Merger.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
14 changes: 7 additions & 7 deletions src/gui/dbsettings/DatabaseSettingsWidgetBrowser.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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);
Expand Down Expand Up @@ -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;
}
}
Expand Down Expand Up @@ -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;
Expand Down

0 comments on commit 48ea2c1

Please sign in to comment.