Fix several cases of incorrect handling of a Group/Entry's LastModification/Creation time

[vuurvli3g]

Recently moved from KeePass original and decided to split my database by moving half of my entries into a new database using the drag & drop mechanism. After completion I came to realize all entries had not only lost their original UUID, but also their original timeinfo.
Relevant issues I could find: #6202, #8170

After analyzing and comparing behavior with KeePass original, the following problems were identified:

• Moving an entry/group within the same database changes their LastModificationTime:
  Cause is the Group/Entry ::setPreviousParentGroupUuid methods fail to protect LastModificationTime against updates.
  Parent changes are tracked by the LocationChanged time and should not additionally be tracked by the LastModificationTime.
• Moving an entry/group across databases results in loss of the original UUID and all timeInfo:
  Instead of using different clone flags for the move operation, the code incorrectly handles it the same way as a copy operation.
  The deletion of the original object causes the UUID(s) to end up on the db's deleted object list. This should be prevented.
  From the discussion it has come forward that the change of UUID is part of the design. The move implementation is specifically designed to support the use-case where multiple databases are in (near) instant sync with each other.
  The change in UUID enables the db to mark the original UUID as deleted and propagate it to the other dbs.
  The loss of CreationTime and LastModificationTime are issues that need to be addressed.
• Copying a group/entry changes their LastModificationTime:
  A copy has the exact same initial state of the "userdata" as the original, where LastModificationTime reflects the point in time when that combination of data was put together. The data without the correct timestamp is a loss of important data.
  The Group/Entry ::clone methods should therefore preserve the LastModificationTime even when the the flag CloneResetTimeInfo is passed.
• Adding/removing an entry to/from a group affects the group's LastModificationTime:
  A change in parent/child relationship is not part of "userdata" and reflecting it with LastModificationTime is incorrect, unnecessary, and brings risk of data-loss on merge!
• Sorting groups resets the LastModificationTime of all groups:
  A change in position of a group is not part of "userdata" and reflecting it with LastModificationTime is incorrect, unnecessary, and brings risk of data-loss on merge!

This PR aims to fix the above issues.

Fixes: #6202
Fixes: #8170

Testing strategy

Added to and adjusted test-cases: TestGroup, TestEntry.
Manual testing by dragging different entries & groups in the same, and between different, databases.

Type of change

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

This is my first PR ever, feedback welcome.

[droidmonkey]

The deletion of the original object causes the UUID(s) to end up on the db's deleted object list. This should be prevented.

This would be a major error. You must mark the uuid as deleted. Otherwise, merge operations will fail. A new UUID is created when moved by design. Otherwise, you would be unable to move it back to the original database.

Perhaps an alternative would be to keep the UUID, but when moving (vs merging), you would check the deleted items list and remove the UUID from that list before committing the move.

[vuurvli3g]

This would be a major error. You must mark the uuid as deleted. Otherwise, merge operations will fail. A new UUID is created when moved by design. Otherwise, you would be unable to move it back to the original database.

This is incorrect.

Changing the UUID is the mistake here. Merging is no longer possible because any other versions of the db will have that entry stored with the original UUID. These entries are therefore no longer related and will be processed as independent entities. Even worse, the original UUID has now been marked as deleted causing these entries to be automatically removed on merge.

Keeping the original UUID on the other hand, does exactly what you would expect it to do. The entry (and UUID) stops existing is the sourceDb and starts existing in the targetDb.
Moving an entry back and forth between databases works as intended and seemingly without problems.
In addition, we have no longer lost the ability to merge.

[droidmonkey]

Ahh I see now, sorry I was mistaken earlier.

[phoerious]

You are missing the case where you are merging the source database with a third one. If you don't add a deleted item for the original uuid, the entry would simply reappear as soon as you sync with the third database. If you move something out of a database, you are effectively deleting it, no matter where it eventually ends up in.

[vuurvli3g]

You are missing the case where you are merging the source database with a third one. If you don't add a deleted item for the original uuid, the entry would simply reappear as soon as you sync with the third database. If you move something out of a database, you are effectively deleting it, no matter where it eventually ends up in.

This is not a problem and expected behavior for a move.
If you move a file from folder A to folder B, would you expect to be able move the same file back to folder A?

I do understand there could be a use-case for wanting to block the ability from an entry to be moved back but that is additional functionality and should not be expected as default behavior for a move.

[droidmonkey]

You are missing the case where you are merging the source database with a third one. If you don't add a deleted item for the original uuid, the entry would simply reappear as soon as you sync with the third database. If you move something out of a database, you are effectively deleting it, no matter where it eventually ends up in.

I can see the argument that moving is more akin to pretending the entry never existed in the source database in the first place.

[phoerious]

This is not a problem and expected behavior for a move.

You misunderstood me. You have three databases, two with the entry, one without. The two with it are synced with each other. If you move the entry to the third database, we need to create a deleted entry for it. Otherwise the sync will recreate it even though we've just moved it out of the database. The expected behaviour would be for the move out/delete to propagate to the other synced database, not for it to be recreated from it.

[vuurvli3g]

No, I did understand you just fine.
Again, what you are describing is simply not a move operation but a "banish entry from database" operation, they are not the same thing!
If I move an entry out of database A into database B and sync (merge) these databases, the entry has to reappear in database A.

[phoerious]

Again, what you are describing is simply not a move operation

It is. If you don't create the deleted entry, you have effectively turned the move operation into a copy operation, because the moved entry will be recreated (more or less immediately). The correct solution would be to keep the UUID intact and ask whether to replace or create a new entry if you move/copy it into a database that has an entry with this UUID already.

Since two databases are not one atomic unit, a move operation is by definition a copy-delete operation.

[vuurvli3g]

Listen I do understand what you are trying to say.

Database A lives on my desktop computer.
Database B lives on my mobile phone.
Database A is synched with database B every day at 12 PM.
I make a new database on my computer, Database C.
I move entryX from database A to (the new) Database C.
Later that day Database A is synched with Database B.
entryX has merged back into database A from database B.

You believe this should not have happened because you moved entryX to database B.

Again, respectfully, I do understand the desire for such functionality, but it is not what a move is supposed to do.
A move here is nothing more and nothing less than a transfer of entryX from storage container A to storage container C.
After the transfer it is as if entryX never existed in A and had always been created in C.

To achieve the functionality your desire, you have to be explicit about it.
Instead of moving entryX to database C you copy (hold CTRL while dragging) the entry, and manual delete the entry in database A.
Now you have explicitly expressed your desire that any and all versions of the entry with the old UUID should be considered void and only the entry with thew new UUID should be tracked from this point forward.

[phoerious]

A move is not what you are describing. A non-atomic move is a copy-delete op. That is expected behaviour and the only behaviour that guarantees consistent results.

[vuurvli3g]

Do you understand that if you move in the way you describe you can no longer merge with other version(s) of that entry that may have had more recent changes than the one you decided to move? All these will end up in the Recycler or worse, be deleted.

You are not moving anything, you are making a look-a-like entry with a new identity that just happens to have the same data.

[phoerious]

Moving something back into the database can be as easy as discarding an existing deleted entry. If that is what you are worried about, then that can be fixed easily. But we cannot just mess with the definition of a move arbitrarily.

[vuurvli3g]

I am afraid I cannot explain it any better.

If you honestly believe that: renaming a file, moving it, adding old filename to a blocklist, would be the definition of a file move operation, than we are going to have to agree to disagree here.

[droidmonkey]

Comparing file operations with database operations is not correct. A database keeps a record of all actions to conduct merging and state keeping correctly. The deleted items list is a record (log) of known uuids that have been removed from the database and should no longer be present.

[phoerious]

It's not a block list. It's a 'this entry has been deleted' marker. Without it, it would be impossible to say whether an entry has been deleted/moved out or is 'just not there yet'. It's totally possible to remove it and put the entry back in place (if it has a newer modification date or if you are performing an explicit copy/move).

[vuurvli3g]

I think I get what you are trying to say.

The current "tracked" move implementation was specifically designed to work for the use-case where 2 databases are in (near) instant sync with each other.
In that case a normal "untracked" move would be useless as the entry would reappear directly after.
While it may not be the only use-case, it makes sense why it was chosen as implementation.

It is clear now this PR was made under a wrong assumption on my part.
I think closing this PR and making 2 seperate PRs would be the way forward?
PR1: fix the mentioned LastModificationTime issues.
PR2: Add new "untracked" move functionality. (I was thinking by holding the Alt key while dragging to another database)

Any thoughts/feedback about the LastModificationTime issues?

[droidmonkey]

Yeah, the modification time issue needs to be fixed. Just eliminate the code changes for the other stuff and re-title this PR. Holding ALT on move is a decent compromise. I would call that "Silent Move" or something in the documentation. Untracked move is good as well. Basically, it moves without recording it.

[vuurvli3g]

Made the relevant changes and added & adjusted test-cases.

I would still like to make an addition to the PR.

Currently, when an entry/group is moved to another db with the tracked move implementation, it gets a new UUID and loses it's original CreationTime as a side-effect.
While CreationTime should ideally reflect the time-of-birth of the UUID, in this particular instance using the original time would seem like the right choice.
There doesn't seem to be any logic that relies on CreationTime in a specific way.

Is there a compelling reason why the original CreationTime isn't used?

[droidmonkey]

You could always change the creation time back after setting the UUID. I do recommend that approach instead of adding a flag to various functions to avoid setting creation time. I believe there is an open issue complaining about the creation time changing on move so I agree with fixing that problem.

[vuurvli3g]

My initial solution was to restore CreationTime manually, but dealing with a group you now have to loop over all sub-groups & entries. While possible, it felt like a dirty workaround. Adding bitflags seemed like the correct solution and would prevent the need for workarounds here and in possible future cases.
The change is transparent to other code and requires no changes to other functions.
Is there fear here there won't be enough bits available in the future or what would you say are the disadvantages to this solution?

[droidmonkey]

Actually good point, I was only thinking of the single entry case. Bitflag it is! (default to setting create time to current)

[droidmonkey]

I think this is too much change for 2.7.8, we will merge this into 2.8.0 baseline only

[vuurvli3g]

Ok thanks.

Some thoughts I had making this PR.
The fixes work but do underline that perhaps some redesign might make sense in the future.
The coupling of updating LastModificationTime and LastAccessTime is not ideal.

The way LastModificationTime is changed makes it prone to (accidentally) update it without accounting for it with a history entry. If updating it was more explicit it could help prevent similar bugs in the future.

LastAccessTime seems kinda arbitrary and somewhat useless currently. Wouldn't it be much nicer to make LastAccessTime reflect the last time an entry was used (auto-type, browser extentions, copied to clipboard)? so let LastAccessTime actually reflect LastUsageTime? I realize this might not according to spec but is this field in its current form actually useful to anyone?

[droidmonkey]

I prefer to drop LastAccessTime as a UI element (column, entry properties tab, etc). It makes no sense at all in the context of a KeePass database. KeePass also does not show this field.