Add CustomData from KDBX4 to Group and Entry by ckieschnick · Pull Request #1477 · keepassxreboot/keepassxc

ckieschnick · 2018-02-12T13:38:54Z

Added support for CustomData from KDBX 4 format in Entry and Group.

Description

Added a CustomData-class to manage custom fields from Metadata, Groups and Entries. Update of timestamps by changing CustomData is implemented through forwarding the modified signal and the self-update-timestamp mechanism which was previously only used in Entry.
Management of CustomData for Group and Entry is similar to KeePass which allows to delete the attributes within the Edit dialog

Motivation and context

Change is required since opening and storing a valid kdbx4-database from KeePass should not loose information. The format version 4 requires to load and store custom data at entries and groups.

How has this been tested?

Added tests to the Testsuite and extended the NewDatabase.xml fixture to contain CustomData at Metadata, Group, Entry and History level.

Types of changes

✅ Bug fix (non-breaking change which fixes an issue)

Checklist:

✅ I have read the CONTRIBUTING document. [REQUIRED]
✅ My code follows the code style of this project. [REQUIRED]
✅ All new and existing tests passed. [REQUIRED]
✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]
✅ I have added tests to cover my changes.

Introduce missing CustomData-attributes of KDBX4 format to allow storing of plugin data for groups and entries - adopt Metadata to use the same storage mechanism Add simple view for CustomData as part of EditWidgetProperties Tracking of CustomData-Modification using SIGNAL-SLOT update-mechanism

Small improvements to existing tests Create test to check modification of CustomData in Entry, Group and Metadata

HistoryItem size calculation uses the correct CustomData object

louib · 2018-02-14T18:56:24Z

src/core/Entry.cpp

    return m_attachments;
 }

+CustomData *Entry::customData()


CustomData* Entry

louib · 2018-02-14T18:56:38Z

src/core/Entry.cpp

+    return m_customData;
+}
+
+const CustomData *Entry::customData() const


CustomData* Entry

louib · 2018-02-14T18:58:48Z

src/core/Entry.h

    EntryAttachments* attachments();
    const EntryAttachments* attachments() const;
+    CustomData *customData();
+    const CustomData *customData() const;


CustomData* customData. There's more occurrences of this in the PR, I did not comment on all of them.

louib · 2018-02-14T19:02:14Z

src/core/Group.cpp

        m_data.isExpanded = expanded;
-        updateTimeinfo();
        if (config()->get("IgnoreGroupExpansion").toBool()) {
+            updateTimeinfo();


@ckieschnick I'm not even sure we should be updating the time info in that case. @phoerious thoughts?

I would say that depends on what KeePass does.

Not quite sure if it is required by KeePass - the change was necessary due to retain the behavior after changing the timestamp update to use the self-listen mechanism with modified-signal (which is explicitly not emitted when "IgnoreGroupExpansion" is on). I think, if the behavior deviates from KeePass, it should be fixed separately.

louib · 2018-02-14T19:06:17Z

src/format/KdbxXmlReader.cpp

            continue;
        }
-
+        if (m_xml.name() == "CustomData" ){


@ckieschnick space before {

And no space before )

louib · 2018-02-14T19:09:09Z

src/format/KdbxXmlWriter.cpp


    writeUuid("LastTopVisibleEntry", group->lastTopVisibleEntry());

+    if (!group->customData()->isEmpty()){


@ckieschnick the check for emptiness could be moved into writeCustomData

Space before {

louib · 2018-02-14T19:14:27Z

tests/data/NewDatabase.xml

 		<CustomData>
 			<Item>
-				<Key>A Sample Test Key</Key>
-				<Value>valu</Value>


@ckieschnick looks like tabs are used in the rest of the file, maybe it would be better to keep it that way for consistency.

louib

@ckieschnick I added some comments, but it looks good in general. Could not test it though, since I'm blocked by #1472.

phoerious · 2018-02-14T22:35:51Z

src/core/CustomData.cpp

@@ -0,0 +1,156 @@
+/*
+ *  Copyright (C) 2012 Felix Geyer <debfx@fobos.de>


Remove this copyright. debfx is the KeePassX maintainer and not involved in KeePassXC.

phoerious · 2018-02-14T22:37:15Z

src/core/CustomData.cpp

+    bool addAttribute = !m_data.contains(key);
+    bool changeValue = !addAttribute && (m_data.value(key) != value);
+
+    if (addAttribute ) {


Remove space before )

phoerious · 2018-02-14T22:39:03Z

src/core/CustomData.cpp

+    bool emitModified = false;
+
+    bool addAttribute = !m_data.contains(key);
+    bool changeValue = !addAttribute && (m_data.value(key) != value);


value() will return a default-constructed element if the key does not exist. Avoid that by checking if the key exists.

The case is already covered since !addAttributes equals m_data.contains(key) - m_data.value(key) is only called when the key exists.

phoerious · 2018-02-14T22:39:55Z

src/core/CustomData.cpp

+
+    if (addAttribute || changeValue) {
+        m_data.insert(key, value);
+        emitModified = true;


Why not emit directly?

I derived the code from an existing class (I think it was EntryAttributes) to reduce style issues. This part was already part of the existing class, so I took it for a code convention.

Just emit the signal right away. There's no point in doing it later.

phoerious · 2018-02-14T22:41:47Z

src/core/CustomData.cpp

+void CustomData::rename(const QString& oldKey, const QString& newKey)
+{
+    if (!m_data.contains(oldKey)) {
+        Q_ASSERT(false);


Avoid Q_ASSERT(false). Replace with

bool exists = m_data.contains(oldKey); Q_ASSERT(exists); if (!exists) { return; ]

phoerious · 2018-02-14T23:02:08Z

src/gui/EditWidgetProperties.h


-    void setFields(TimeInfo timeInfo, Uuid uuid);
+    void setFields(const TimeInfo &timeInfo, const Uuid &uuid);
+    void setCustomData(const CustomData *customData);


const TimeInfo&
const Uuid&
CustomData*

phoerious · 2018-02-14T23:02:59Z

tests/TestKeePass2Format.cpp

    entry->setPassword(QString::fromUtf8("\xc3\xa4\xa3\xb6\xc3\xbc\xe9\x9b\xbb\xe7\xb4\x85"));
    entry->setUuid(Uuid::random());
    entry->attributes()->set("test", "protectedTest", true);
+    entry->customData()->set("CustomDataEntryKey", "CustomDataValue" );


Remove space before )

phoerious · 2018-02-14T23:04:15Z

tests/TestModified.cpp

    entry->endUpdate();
    QCOMPARE(entry->historyItems().size(), ++historyItemsSize);
-    historyEntry = entry->historyItems().at(historyItemsSize - 1);
+    Entry *historyEntry = entry->historyItems().at(historyItemsSize - 1);


phoerious · 2018-02-14T23:04:42Z

tests/TestModified.cpp

+void TestModified::testCustomData()
+{
+    int spyCount = 0;
+    Database* db = new Database();


Use QScopedPointer or allocate on the stack

phoerious · 2018-02-14T23:05:23Z

tests/TestModified.cpp

+
+    Group* group = new Group();
+    group->setParent(root);
+    Entry* entry = new Entry();


auto* for all these to avoid class name repetition.

louib · 2018-02-15T15:13:32Z

src/core/CustomData.cpp

+
+void CustomData::copyDataFrom(const CustomData* other)
+{
+    if (*this != *other) {


@ckieschnick is this really a possibility? shouldn't we assert this doesn't happen instead?

I'm not that familiar with the KeePassXC code. Skimming the code, I think an assert would be correct - but the check it is a pattern in different data classes (EntryAttributes, EntryAttachments) which suggests that there is or there was a case of self assignment (since CustomData are used in Entry, Group and Metadata the risk is even higher). So I pass the question back to you.

I would add an assert in addition to the check. The assert is for debug purposes only.

Fixed several code style issues (whitespace, position of * and &, assert style) Added several QPointer and QScopedPointer where appropriate

phoerious · 2018-02-16T17:49:13Z

src/format/KdbxXmlReader.cpp

 }

-void KdbxXmlReader::parseCustomData()
+void KdbxXmlReader::parseCustomData(CustomData *customData)


You missed a few of these.

phoerious · 2018-02-16T17:49:54Z

src/gui/EditWidgetProperties.cpp

+            const QString key = index.data().toString();
+            m_customData->remove(key);
+        }
+        this->updateModel();


Also remove the explicit this pointer (I might have missed that last time).

phoerious · 2018-02-18T15:37:00Z

Since I can't push to the private source repository, please continue here: #1494

Christian Kieschnick added 3 commits February 12, 2018 12:37

Add tests to check CustomData modification

4e694cf

Small improvements to existing tests Create test to check modification of CustomData in Entry, Group and Metadata

Fix for HistoryItem size calculation

70c1689

HistoryItem size calculation uses the correct CustomData object

TheZ3ro requested review from TheZ3ro, droidmonkey and phoerious February 12, 2018 14:03

phoerious added this to the v2.3.0 milestone Feb 12, 2018

phoerious added the file format label Feb 12, 2018

louib reviewed Feb 14, 2018

View reviewed changes

src/core/Entry.cpp Outdated

return m_customData;

}

const CustomData *Entry::customData() const

Copy link
Copy Markdown

Member

louib Feb 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CustomData* Entry

louib reviewed Feb 14, 2018

View reviewed changes

louib requested changes Feb 14, 2018

View reviewed changes

phoerious requested changes Feb 14, 2018

View reviewed changes

louib reviewed Feb 15, 2018

View reviewed changes

Implement feedback

87de61a

Fixed several code style issues (whitespace, position of * and &, assert style) Added several QPointer and QScopedPointer where appropriate

phoerious reviewed Feb 16, 2018

View reviewed changes

phoerious mentioned this pull request Feb 18, 2018

Add support for custom data #1494

Merged

phoerious closed this Feb 18, 2018

ckieschnick mentioned this pull request Feb 19, 2018

Implement feedback for CustomData #1496

Closed


		writeUuid("LastTopVisibleEntry", group->lastTopVisibleEntry());

		if (!group->customData()->isEmpty()){

		@@ -0,0 +1,156 @@
		/*
		* Copyright (C) 2012 Felix Geyer <debfx@fobos.de>

Uh oh!

Conversation

ckieschnick commented Feb 12, 2018

Description

Motivation and context

How has this been tested?

Types of changes

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

louib Feb 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

louib left a comment

Choose a reason for hiding this comment

Uh oh!

phoerious Feb 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phoerious Feb 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phoerious commented Feb 18, 2018

Uh oh!

louib Feb 14, 2018 •

edited

Loading

phoerious Feb 14, 2018 •

edited

Loading

phoerious Feb 16, 2018 •

edited

Loading