Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Database Custom Data Merging #3002

Merged

Conversation

varjolintu
Copy link
Member

@varjolintu varjolintu commented Apr 13, 2019

Type of change

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

Description and Context

Merges the custom data from database metadata.

We store an extra key _LAST_MODIFIED (UTC Date/Time) and make comparisons based on that value. Lacking this key we assume the source DB is "newer".

If Source is Newer:

  • New Key -> Add to Target
  • Missing Key -> Delete from Target
  • Existing Key -> Replace Value on Target

If Source is Older:

  • Don't modify target data

Fixes #2991.

Testing strategy

Test cases are made.

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.

src/core/Merger.cpp Outdated Show resolved Hide resolved
src/core/Merger.cpp Outdated Show resolved Hide resolved
@droidmonkey droidmonkey changed the base branch from develop to release/2.4.2 April 15, 2019 18:21
@droidmonkey
Copy link
Member

I'd like to get this into 2.4.2

@varjolintu varjolintu force-pushed the merge_custom_data branch 3 times, most recently from 01ecb77 to dcb18b1 Compare April 18, 2019 08:50
@varjolintu
Copy link
Member Author

I know the tests still fail. This is because CustomData now has an extra size. And the returned size cannot be changed in a way the _LAST_MODIFIED should stay hidden. If returned size/dataSize is modified to hide _LAST_MODIFIED, tests will fail when the KDBX format is upgraded to version 4.

@droidmonkey
Copy link
Member

It would be inappropriate to "hide" the last modified entry. The tests need to be adjusted.

@varjolintu
Copy link
Member Author

@droidmonkey That's what I'm doing now. For example TestKdbx4.cpp:189 still fails.

@droidmonkey
Copy link
Member

I'll look at it when I can

@varjolintu
Copy link
Member Author

varjolintu commented Apr 19, 2019

I wonder if this happens because KdbxXmlReader (and other classes) uses CustomData::set() when parsing items and now it updates the _LAST_MODIFIED every time (KdbxXmlReader.cpp:438 for example).

@phoerious
Copy link
Member

The alternative would be to use the timestamp as a suffix of the key and parse it out before displaying the key. But that may be a bit ugly if viewed with anything but KeePassXC.

@droidmonkey
Copy link
Member

Yah that would also break plugins in other programs that were unaware of the suffix.

@varjolintu
Copy link
Member Author

The format clearly restricts the possibilities here.

@phoerious
Copy link
Member

The proper way would be an XML attribute or another element (I don't like this element-centric style, but KDBX uses it everywhere). We need to keep that in the list for KDBX5.

Copy link
Member

@droidmonkey droidmonkey left a comment

Choose a reason for hiding this comment

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

You need to update the other tests that check the custom data size to pass.

@varjolintu
Copy link
Member Author

TestKdbx4.cpp:189 is the only thing failing, but I haven't found a reason why it happens. The content is identical.

@droidmonkey
Copy link
Member

It's failing because the number of custom data elements is 1 higher then expected since the _LAST_MODIFIED key is added.

@varjolintu
Copy link
Member Author

@droidmonkey That's not the case in that comparison. The number of custom data elements is identical. I already changed the sizes. Should comment them though that the +1 is because of _LAST_MODIFIED.

@droidmonkey droidmonkey changed the title Merge custom data Support Database Custom Data Merging May 1, 2019
@droidmonkey droidmonkey merged commit e4eee89 into keepassxreboot:release/2.4.2 May 1, 2019
droidmonkey added a commit that referenced this pull request May 31, 2019
- Improve resilience against memory attacks - overwrite memory before free [#3020]
- Prevent infinite save loop when location is unavailable [#3026]
- Attempt to fix quitting application when shutdown or logout issued [#3199]
- Support merging database custom data [#3002]
- Fix opening URL's with non-http schemes [#3153]
- Fix data loss due to not reading all database attachments if duplicates exist [#3180]
- Fix entry context menu disabling when using keyboard navigation [#3199]
- Fix behaviors when canceling an entry edit [#3199]
- Fix processing of tray icon click and doubleclick [#3112]
- Update group in preview widget when focused [#3199]
- Prefer DuckDuckGo service over direct icon download (increases resolution) [#2996]
- Remove apply button in application settings [#3019]
- Use winqtdeploy on Windows to correct deployment issues [#3025]
- Don't mark entry edit as modified when attribute selection changes [#3041]
- Use console code page CP_UTF8 on Windows if supported [#3050]
- Snap: Fix locking database with session lock [#3046]
- Snap: Fix theming across Linux distributions [#3057]
- Snap: Use SNAP_USER_COMMON and SNAP_USER_DATA directories [#3131]
- KeeShare: Automatically enable WITH_XC_KEESHARE_SECURE if quazip is found [#3088]
- macOS: Fix toolbar text when in dark mode [#2998]
- macOS: Lock database on switching user [#3097]
- macOS: Fix global Auto-Type when the database is locked [#3138]
- Browser: Close popups when database is locked [#3093]
- Browser: Add tests [#3016]
- Browser: Don't create default group if custom group is enabled [#3127]
@varjolintu varjolintu deleted the merge_custom_data branch June 1, 2019 05:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants