Add support for custom data by phoerious · Pull Request #1494 · keepassxreboot/keepassxc

phoerious · 2018-02-18T15:36:04Z

Description

Adds support for custom plugin data, which were previously discarded silently.

This is the same as #1477, but with these additional fixes (I can't push to the original PR, because the repository is private):

properly read and write custom header data
use QVariant instead of QString for custom data values
ensure that adding custom data upgrades to KDBX 4
move custom data tests from KeePass2Format to Kdbx4 tests to not interfere with Kdbx3 tests
add vertical spacer below "Remove" button in properties edit widget
implement remaining review comments (except assertion in copyDataFrom(), since we only compare if values and not pointers are the same and copying from an entry with the same contents is not an error)

Types of changes

✅ New feature (non-breaking change which adds functionality)

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

varjolintu · 2018-02-19T09:41:39Z

FYI: the Remove button doesn't have a confirmation when removing custom data. Removing attributes does that. Should that be done here too?

ckieschnick · 2018-02-19T09:58:13Z

KeePass2 does not pop up any dialog - but this is may change in the future. Sounds like a good idea since removing data is potentially tinkering with the data of some plugin which may lead to unexpected behavior.

ckieschnick · 2018-02-19T10:02:13Z

src/core/CustomData.cpp

    while (i.hasNext()) {
        i.next();
-        size += i.key().toUtf8().size() + i.value().toUtf8().size();
+        size += i.key().toUtf8().size() + i.value().toString().toUtf8().size();


You may look at this again - QVariant::toString() does give the correct size for arbitrary variants afaik. To calculate the correct size, one should cast to the correct type and than calculate the size for the specific type. KeePass2 does implement CustomData with a StringDictionary so I would suggest going back to QString

ckieschnick

Please rethink the change of CustomData from QString to QVariant since this does not match the use in KeePass2.

phoerious · 2018-02-19T11:34:33Z

KeePass2 uses a StringDictionary for custom entry/group data, but a VariantDictionary for custom header data. A variant map can do both, a string map can only be used for string data.

ckieschnick · 2018-02-19T11:44:41Z

Do you want to merge the data container for PulicCustomData and CustomData? PublicCustomData is currently a variant dict, but when I interpret the KeePass2 source code comment correctly, it is not encouraged to use this for plugin development since the data is not encrypted (and not exported to xml). I would suggest to keep both separate - the Group and Entry both have CustomData while the database (Metadata) has both - PublicCustomData and CustomData. From my point of view, moving CustomData makes the API for the developer more complex since one has to remember if arbitrary types are allowed (PublicCustomData) or it is string only (CustomData).

phoerious · 2018-02-19T11:57:27Z

This is again where the KDBX4 specification is incomplete and totally misleading. I didn't realize that a database has both custom data and public custom data. I think we really need to work on a specification document ourselves. It is so annoying when I need to browse the KeePass source code only to understand the spec. If those are two fields, I think it makes sense to restrict it to QString and then use a plain QVariantMap for public custom data (although it's possible to use arbitrary data types for the XML stuff, too, which are then automatically converted to QString).

phoerious · 2018-02-19T12:43:53Z

I updated the PR to properly handle both public custom data and custom data on database level. Please have a look.

phoerious · 2018-02-19T14:25:06Z

@varjolintu Fixed. I also enable the button only when a row is selected and I activated column headers.

ckieschnick

Please look at my annotations, not sure if I introduced the error, but entries in Metadata::customData() are supported in formats below Kdbx4.

ckieschnick · 2018-02-19T15:53:59Z

src/format/KeePass2Writer.cpp


    // determine KDBX3 vs KDBX4
-    if (db->kdf()->uuid() == KeePass2::KDF_AES_KDBX3 && db->publicCustomData().isEmpty()) {
+    bool hasCustomData = !db->publicCustomData().isEmpty() || (db->metadata()->customData() && !db->metadata()->customData()->isEmpty());


Entries in Metadata::customData() do not force KeePass2::FILE_VERSION_4. Just entries within Database::publicCustomData(), Entry::customData() and Group::customData() force the new format .

ckieschnick · 2018-02-19T15:56:21Z

src/format/KdbxXmlWriter.cpp

        writeBinaries();
    }
-    writeCustomData();
+    if (m_kdbxVersion >= KeePass2::FILE_VERSION_4) {


Metadata::customData() was supported in versions below KeePass2::FILE_VERSION_4.

True. I verified it by exporting an XML database with KeePass 2.34.
I think now we got everything the way KeePass does. Again, KDBX4 spec is pretty useless.

droidmonkey

Looks great, couple changes

droidmonkey · 2018-02-19T23:27:14Z

src/core/CustomData.cpp

+    emit modified();
+}
+
+void CustomData::rename(const QString& oldKey, const QString& newKey)


This function is never used and I don't see a case for it either. Custom Data is not directly user editable. Recommend remove.

It may be useful for plugins, but I don't know how often a plugin needs to rename a field.

droidmonkey · 2018-02-19T23:27:46Z

src/core/CustomData.cpp

+    return m_data.contains(key);
+}
+
+bool CustomData::containsValue(const QString& value) const


Function never used, recommend remove.

I removed both.

droidmonkey · 2018-02-19T23:30:34Z

src/core/Entry.cpp

        entry->m_uuid = m_uuid;
    }
    entry->m_data = m_data;
+    entry->m_customData->copyDataFrom(m_customData);


Shouldn't we just call entry->copyDataFrom(this) for all these since we are duplicating code from below.

Very good point. Fixed.

droidmonkey · 2018-02-19T23:36:08Z

src/core/Group.cpp

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


Wouldn't you want to do this AFTER the conditional (or not at all)? I'd imagine if you were ignoring group expansion you wouldn't want to update the time info.

After the conditional, the modified()-self-updating mechanism does update the timestamp. The conditional does a short-cut return, explicitly preventing the modified() signal, so a manual update was necessary to keep the existing behavior. I would suggest to move this discussion to another ticket since it is not related to the issue.

I removed it. It contradicts the purpose of that setting and calling updateTimeinfo() without emitting modified() makes no sense anyway.

droidmonkey · 2018-02-19T23:38:01Z

src/core/Group.h

    QList<Entry*> m_entries;

+    QPointer<CustomData> m_customData;
+


Remove extra space

droidmonkey · 2018-02-19T23:45:24Z

tests/TestKdbx4.cpp

+        QVariantMap publicCustomData;
+        publicCustomData.insert("CustomPublicData", "Hey look, I turned myself into a pickle!");
+        sourceDb->setPublicCustomData(publicCustomData);
+        sourceDb->rootGroup()->customData()->set("CustomGroupData", "I just killed my family! I don't care who they were!");


A little morbid...

It's a quote. :-D

Gotcha, I don't watch Rick and Morty... although maybe I should!

You should.

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

Ensure adding custom data upgrades to KDBX4 Implement review feedback

…data selected

- Add support for KDBX 4.0, Argon2 and ChaCha20 [#148, #1179, #1230, #1494] - Add SSH Agent feature [#1098, #1450, #1463] - Add preview panel with details of the selected entry [#879, #1338] - Add more and configurable columns to entry table and allow copying of values by double click [#1305] - Add KeePassXC-Browser API as a replacement for KeePassHTTP [#608] - Deprecate KeePassHTTP [#1392] - Add support for Steam one-time passwords [#1206] - Add support for multiple Auto-Type sequences for a single entry [#1390] - Adjust YubiKey HMAC-SHA1 challenge-response key generation for KDBX 4.0 [#1060] - Replace qHttp with cURL for website icon downloads [#1460] - Remove lock file [#1231] - Add option to create backup file before saving [#1385] - Ask to save a generated password before closing the entry password generator [#1499] - Resolve placeholders recursively [#1078] - Add Auto-Type button to the toolbar [#1056] - Improve window focus handling for Auto-Type dialogs [#1204, #1490] - Auto-Type dialog and password generator can now be exited with ESC [#1252, #1412] - Add optional dark tray icon [#1154] - Add new "Unsafe saving" option to work around saving problems with file sync services [#1385] - Add IBus support to AppImage and additional image formats to Windows builds [#1534, #1537] - Add diceware password generator to CLI [#1406] - Add --key-file option to CLI [#816, #824] - Add DBus interface for opening and closing KeePassXC databases [#283] - Add KDBX compression options to database settings [#1419] - Discourage use of old fixed-length key files in favor of arbitrary files [#1326, #1327] - Correct reference resolution in entry fields [#1486] - Fix window state and recent databases not being remembered on exit [#1453] - Correct history item generation when configuring TOTP for an entry [#1446] - Correct multiple TOTP bugs [#1414] - Automatic saving after every change is now a default [#279] - Allow creation of new entries during search [#1398] - Correct menu issues on macOS [#1335] - Allow compilation on OpenBSD [#1328] - Improve entry attachments view [#1139, #1298] - Fix auto lock for Gnome and Xfce [#910, #1249] - Don't remember key files in file dialogs when the setting is disabled [#1188] - Improve database merging and conflict resolution [#807, #1165] - Fix macOS pasteboard issues [#1202] - Improve startup times on some platforms [#1205] - Hide the notes field by default [#1124] - Toggle main window by clicking tray icon with the middle mouse button [#992] - Fix custom icons not copied over when databases are merged [#1008] - Allow use of DEL key to delete entries [#914] - Correct intermittent crash due to stale history items [#1527] - Sanitize newline characters in title, username and URL fields [#1502] - Reopen previously opened databases in correct order [#774] - Use system's zxcvbn library if available [#701] - Implement various i18n improvements [#690, #875, #1436]

phoerious added the file format label Feb 18, 2018

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

phoerious requested a review from a team February 18, 2018 15:36

phoerious mentioned this pull request Feb 18, 2018

Add CustomData from KDBX4 to Group and Entry #1477

Closed

phoerious force-pushed the feature/custom-data branch 7 times, most recently from 8e5c201 to 616732d Compare February 18, 2018 20:26

phoerious mentioned this pull request Feb 18, 2018

Use custom data to save browser connection keys keepassxreboot/keepassxc-browser#11

Closed

phoerious force-pushed the feature/custom-data branch from 616732d to 1e4dfa0 Compare February 19, 2018 08:42

ckieschnick mentioned this pull request Feb 19, 2018

Implement feedback for CustomData #1496

Closed

ckieschnick reviewed Feb 19, 2018

View reviewed changes

ckieschnick suggested changes Feb 19, 2018

View reviewed changes

varjolintu mentioned this pull request Feb 19, 2018

Use custom data to save any browser integration related data #1497

Merged

phoerious force-pushed the feature/custom-data branch from 1e4dfa0 to 0425d20 Compare February 19, 2018 12:43

phoerious force-pushed the feature/custom-data branch 3 times, most recently from 9901683 to 94ec995 Compare February 19, 2018 13:14

phoerious force-pushed the feature/custom-data branch from 9a32448 to 202338a Compare February 19, 2018 14:43

ckieschnick suggested changes Feb 19, 2018

View reviewed changes

phoerious force-pushed the feature/custom-data branch 3 times, most recently from 1bff019 to 64bb614 Compare February 19, 2018 21:24

droidmonkey requested changes Feb 19, 2018

View reviewed changes

phoerious force-pushed the feature/custom-data branch 3 times, most recently from e6af788 to 5691806 Compare February 21, 2018 08:40

droidmonkey approved these changes Feb 21, 2018

View reviewed changes

Christian Kieschnick and others added 5 commits February 21, 2018 13:16

Add CustomData regression tests

5ff4a17

Properly save custom header data

96fb5b2

Ensure adding custom data upgrades to KDBX4 Implement review feedback

Add additional tests for saving and reading KDBX files with custom data

a22ba84

Ask user before deleting custom plugin data and disable button if no …

f4246d9

…data selected

phoerious force-pushed the feature/custom-data branch from 5691806 to f4246d9 Compare February 21, 2018 12:16

phoerious merged commit f15088f into release/2.3.0 Feb 21, 2018

phoerious deleted the feature/custom-data branch February 21, 2018 12:23

Uh oh!

Conversation

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

Uh oh!

Description

Types of changes

Checklist:

Uh oh!

varjolintu commented Feb 19, 2018

Uh oh!

ckieschnick commented Feb 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ckieschnick left a comment

Choose a reason for hiding this comment

Uh oh!

phoerious commented Feb 19, 2018

Uh oh!

ckieschnick commented Feb 19, 2018

Uh oh!

phoerious commented Feb 19, 2018

Uh oh!

phoerious commented Feb 19, 2018

Uh oh!

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

Uh oh!

Uh oh!

ckieschnick left a comment

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!

droidmonkey left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

phoerious commented Feb 18, 2018 •

edited

Loading

phoerious commented Feb 19, 2018 •

edited

Loading