Implement KDBX 4.1 CustomData modification date

We keep the old merging behaviour for now, since deleting a
CustomData entry does not create DeletedObject.

src/core/CustomData.cpp

@@ -26,6 +26,9 @@ const QString CustomData::BrowserKeyPrefix = QStringLiteral("KPXC_BROWSER_");
 const QString CustomData::BrowserLegacyKeyPrefix = QStringLiteral("Public Key: ");
 const QString CustomData::ExcludeFromReportsLegacy = QStringLiteral("KnownBad");
 
+// Fallback item for return by reference
+static const CustomData::CustomDataItem NULL_ITEM;
+
 CustomData::CustomData(QObject* parent)
     : ModifiableObject(parent)
 {
@@ -43,7 +46,17 @@ bool CustomData::hasKey(const QString& key) const
 
 QString CustomData::value(const QString& key) const
 {
-    return m_data.value(key);
+    return m_data.value(key).value;
+}
+
+const CustomData::CustomDataItem& CustomData::item(const QString& key) const
+{
+    auto item = m_data.find(key);
+    Q_ASSERT(item != m_data.end());
+    if (item == m_data.end()) {
+        return NULL_ITEM;
+    }
+    return item.value();
 }
 
 bool CustomData::contains(const QString& key) const
@@ -53,20 +66,28 @@ bool CustomData::contains(const QString& key) const
 
 bool CustomData::containsValue(const QString& value) const
 {
-    return asConst(m_data).values().contains(value);
+    for (auto i = m_data.constBegin(); i != m_data.constEnd(); ++i) {
+        if (i.value().value == value) {
+            return true;
+        }
+    }
+    return false;
 }
 
-void CustomData::set(const QString& key, const QString& value)
+void CustomData::set(const QString& key, CustomDataItem item)
 {
     bool addAttribute = !m_data.contains(key);
-    bool changeValue = !addAttribute && (m_data.value(key) != value);
+    bool changeValue = !addAttribute && (m_data.value(key).value != item.value);
 
     if (addAttribute) {
         emit aboutToBeAdded(key);
     }
 
+    if (!item.lastModified.isValid()) {
+        item.lastModified = Clock::currentDateTimeUtc();
+    }
     if (addAttribute || changeValue) {
-        m_data.insert(key, value);
+        m_data.insert(key, item);
         updateLastModified();
         emitModified();
     }
@@ -76,6 +97,11 @@ void CustomData::set(const QString& key, const QString& value)
     }
 }
 
+void CustomData::set(const QString& key, const QString& value, const QDateTime& lastModified)
+{
+    set(key, {value, lastModified});
+}
+
 void CustomData::remove(const QString& key)
 {
     emit aboutToBeRemoved(key);
@@ -98,11 +124,12 @@ void CustomData::rename(const QString& oldKey, const QString& newKey)
         return;
     }
 
-    QString data = value(oldKey);
+    CustomDataItem data = m_data.value(oldKey);
 
     emit aboutToRename(oldKey, newKey);
 
     m_data.remove(oldKey);
+    data.lastModified = Clock::currentDateTimeUtc();
     m_data.insert(newKey, data);
 
     updateLastModified();
@@ -125,15 +152,41 @@ void CustomData::copyDataFrom(const CustomData* other)
     emitModified();
 }
 
-QDateTime CustomData::getLastModified() const
+QDateTime CustomData::lastModified() const
 {
     if (m_data.contains(LastModified)) {
-        return Clock::parse(m_data.value(LastModified));
+        return Clock::parse(m_data.value(LastModified).value);
     }
-    return {};
+
+    // Try to find the latest modification time in items as a fallback
+    QDateTime modified;
+    for (auto i = m_data.constBegin(); i != m_data.constEnd(); ++i) {
+        if (i->lastModified.isValid() && (!modified.isValid() || i->lastModified > modified)) {
+            modified = i->lastModified;
+        }
+    }
+    return modified;
 }
 
-bool CustomData::isProtectedCustomData(const QString& key) const
+QDateTime CustomData::lastModified(const QString& key) const
+{
+    return m_data.value(key).lastModified;
+}
+
+void CustomData::updateLastModified(QDateTime lastModified)
+{
+    if (m_data.isEmpty() || (m_data.size() == 1 && m_data.contains(LastModified))) {
+        m_data.remove(LastModified);
+        return;
+    }
+
+    if (!lastModified.isValid()) {
+        lastModified = Clock::currentDateTimeUtc();
+    }
+    m_data.insert(LastModified, {lastModified.toString(), QDateTime()});
+}
+
+bool CustomData::isProtected(const QString& key) const
 {
     return key.startsWith(CustomData::BrowserKeyPrefix) || key.startsWith(CustomData::Created);
 }
@@ -172,20 +225,15 @@ int CustomData::dataSize() const
 {
     int size = 0;
 
-    QHashIterator<QString, QString> i(m_data);
+    QHashIterator<QString, CustomDataItem> i(m_data);
     while (i.hasNext()) {
         i.next();
-        size += i.key().toUtf8().size() + i.value().toUtf8().size();
+
+        // In theory, we should be adding the datetime string size as well, but it makes
+        // length calculations rather unpredictable. We also don't know if this instance
+        // is entry/group-level CustomData or global CustomData (the only CustomData that
+        // actually retains the datetime in the KDBX file).
+        size += i.key().toUtf8().size() + i.value().value.toUtf8().size();
     }
     return size;
 }
-
-void CustomData::updateLastModified()
-{
-    if (m_data.isEmpty() || (m_data.size() == 1 && m_data.contains(LastModified))) {
-        m_data.remove(LastModified);
-        return;
-    }
-
-    m_data.insert(LastModified, Clock::currentDateTimeUtc().toString());
-}

src/core/CustomData.h

@@ -18,6 +18,7 @@
 #ifndef KEEPASSXC_CUSTOMDATA_H
 #define KEEPASSXC_CUSTOMDATA_H
 
+#include <QDateTime>
 #include <QHash>
 #include <QObject>
 
@@ -28,13 +29,30 @@ class CustomData : public ModifiableObject
     Q_OBJECT
 
 public:
+    struct CustomDataItem
+    {
+        QString value;
+        QDateTime lastModified;
+
+        bool inline operator==(const CustomDataItem& rhs) const
+        {
+            // Compare only actual values, not modification dates
+            return value == rhs.value;
+        }
+    };
+
     explicit CustomData(QObject* parent = nullptr);
     QList<QString> keys() const;
     bool hasKey(const QString& key) const;
     QString value(const QString& key) const;
+    const CustomDataItem& item(const QString& key) const;
     bool contains(const QString& key) const;
     bool containsValue(const QString& value) const;
-    void set(const QString& key, const QString& value);
+    QDateTime lastModified() const;
+    QDateTime lastModified(const QString& key) const;
+    bool isProtected(const QString& key) const;
+    void set(const QString& key, CustomDataItem item);
+    void set(const QString& key, const QString& value, const QDateTime& lastModified = {});
     void remove(const QString& key);
     void rename(const QString& oldKey, const QString& newKey);
     void clear();
@@ -42,16 +60,17 @@ public:
     int size() const;
     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;
 
+    // Pre-defined keys
     static const QString LastModified;
     static const QString Created;
     static const QString BrowserKeyPrefix;
     static const QString BrowserLegacyKeyPrefix;
-    static const QString ExcludeFromReportsLegacy; // Pre-KDBX 4.1
+
+    // Pre-KDBX 4.1
+    static const QString ExcludeFromReportsLegacy;
 
 signals:
     void aboutToBeAdded(const QString& key);
@@ -64,10 +83,10 @@ signals:
     void reset();
 
 private slots:
-    void updateLastModified();
+    void updateLastModified(QDateTime lastModified = {});
 
 private:
-    QHash<QString, QString> m_data;
+    QHash<QString, CustomDataItem> m_data;
 };
 
 #endif // KEEPASSXC_CUSTOMDATA_H

src/core/Merger.cpp

@@ -617,8 +617,8 @@ Merger::ChangeList Merger::mergeMetadata(const MergeContext& context)
     }
 
     // Merge Custom Data if source is newer
-    const auto targetCustomDataModificationTime = targetMetadata->customData()->getLastModified();
-    const auto sourceCustomDataModificationTime = sourceMetadata->customData()->getLastModified();
+    const auto targetCustomDataModificationTime = targetMetadata->customData()->lastModified();
+    const auto sourceCustomDataModificationTime = sourceMetadata->customData()->lastModified();
     if (!targetMetadata->customData()->contains(CustomData::LastModified)
         || (targetCustomDataModificationTime.isValid() && sourceCustomDataModificationTime.isValid()
             && targetCustomDataModificationTime < sourceCustomDataModificationTime)) {
@@ -628,8 +628,7 @@ Merger::ChangeList Merger::mergeMetadata(const MergeContext& context)
         // Check missing keys from source. Remove those from target
         for (const auto& key : targetCustomDataKeys) {
             // Do not remove protected custom data
-            if (!sourceMetadata->customData()->contains(key)
-                && !sourceMetadata->customData()->isProtectedCustomData(key)) {
+            if (!sourceMetadata->customData()->contains(key) && !sourceMetadata->customData()->isProtected(key)) {
                 auto value = targetMetadata->customData()->value(key);
                 targetMetadata->customData()->remove(key);
                 changes << tr("Removed custom data %1 [%2]").arg(key, value);

src/format/KdbxXmlReader.cpp

@@ -420,7 +420,7 @@ void KdbxXmlReader::parseCustomDataItem(CustomData* customData)
     Q_ASSERT(m_xml.isStartElement() && m_xml.name() == "Item");
 
     QString key;
-    QString value;
+    CustomData::CustomDataItem item;
     bool keySet = false;
     bool valueSet = false;
 
@@ -429,15 +429,17 @@ void KdbxXmlReader::parseCustomDataItem(CustomData* customData)
             key = readString();
             keySet = true;
         } else if (m_xml.name() == "Value") {
-            value = readString();
+            item.value = readString();
             valueSet = true;
+        } else if (m_xml.name() == "LastModificationTime") {
+            item.lastModified = readDateTime();
         } else {
             skipCurrentElement();
         }
     }
 
     if (keySet && valueSet) {
-        customData->set(key, value);
+        customData->set(key, item);
         return;
     }
 

src/format/KdbxXmlWriter.cpp

@@ -131,7 +131,7 @@ void KdbxXmlWriter::writeMetadata()
     if (m_kdbxVersion < KeePass2::FILE_VERSION_4) {
         writeBinaries();
     }
-    writeCustomData(m_meta->customData());
+    writeCustomData(m_meta->customData(), true);
 
     m_xml.writeEndElement();
 }
@@ -220,7 +220,7 @@ void KdbxXmlWriter::writeBinaries()
     m_xml.writeEndElement();
 }
 
-void KdbxXmlWriter::writeCustomData(const CustomData* customData)
+void KdbxXmlWriter::writeCustomData(const CustomData* customData, bool writeItemLastModified)
 {
     if (customData->isEmpty()) {
         return;
@@ -229,18 +229,23 @@ void KdbxXmlWriter::writeCustomData(const CustomData* customData)
 
     const QList<QString> keyList = customData->keys();
     for (const QString& key : keyList) {
-        writeCustomDataItem(key, customData->value(key));
+        writeCustomDataItem(key, customData->item(key), writeItemLastModified);
     }
 
     m_xml.writeEndElement();
 }
 
-void KdbxXmlWriter::writeCustomDataItem(const QString& key, const QString& value)
+void KdbxXmlWriter::writeCustomDataItem(const QString& key,
+                                        const CustomData::CustomDataItem& item,
+                                        bool writeLastModified)
 {
     m_xml.writeStartElement("Item");
 
     writeString("Key", key);
-    writeString("Value", value);
+    writeString("Value", item.value);
+    if (writeLastModified && m_kdbxVersion >= KeePass2::FILE_VERSION_4 && item.lastModified.isValid()) {
+        writeDateTime("LastModificationTime", item.lastModified);
+    }
 
     m_xml.writeEndElement();
 }

src/format/KdbxXmlWriter.h

@@ -21,6 +21,7 @@
 #include <QDateTime>
 #include <QXmlStreamWriter>
 
+#include "core/CustomData.h"
 #include "core/Group.h"
 #include "core/Metadata.h"
 
@@ -49,8 +50,9 @@ private:
     void writeCustomIcons();
     void writeIcon(const QUuid& uuid, const Metadata::CustomIconData& iconData);
     void writeBinaries();
-    void writeCustomData(const CustomData* customData);
-    void writeCustomDataItem(const QString& key, const QString& value);
+    void writeCustomData(const CustomData* customData, bool writeItemLastModified = false);
+    void
+    writeCustomDataItem(const QString& key, const CustomData::CustomDataItem& item, bool writeLastModified = false);
     void writeRoot();
     void writeGroup(const Group* group);
     void writeTimes(const TimeInfo& ti);

tests/TestKdbx4.cpp

@@ -373,7 +373,7 @@ void TestKdbx4Argon2::testCustomData()
     db.metadata()->customData()->set(customDataKey1, customData1);
     db.metadata()->customData()->set(customDataKey2, customData2);
     auto lastModified = db.metadata()->customData()->value(CustomData::LastModified);
-    const int dataSize = customDataKey1.toUtf8().size() + customDataKey1.toUtf8().size() + customData1.toUtf8().size()
+    const int dataSize = customDataKey1.toUtf8().size() + customDataKey2.toUtf8().size() + customData1.toUtf8().size()
                          + customData2.toUtf8().size() + lastModified.toUtf8().size()
                          + CustomData::LastModified.toUtf8().size();
     QCOMPARE(db.metadata()->customData()->size(), 3);