keepassxreboot / keepassxc

KeePassXC is a cross-platform community-driven port of the Windows application “Keepass Password Safe”.
https://keepassxc.org/
Other
21.32k stars 1.47k forks source link

Recycle Bin is Imported using KeeShare #2798

Open droidmonkey opened 5 years ago

droidmonkey commented 5 years ago

Expected Behavior

The recycle bin and entries in the recycle bin are not imported or exported when using KeeShare. This would be an option in the KeeShare settings of the group.

Current Behavior

Recycle Bin is fully synchronized.

image

mstarke commented 5 years ago

My initial thought on this is it to let the user choose what should happen and default to "do not synchronise". It might be useful to warn if the user is sharing a recycle bin. This is sort of an edge case since it only happens when the root group is shared or the recycle bin is placed inside a shared group.

droidmonkey commented 5 years ago

Yes agree completely

tideg commented 5 years ago

But how can deleted entries be synchronized at all if not via a shared recycle bin? Currently, there is no way to remove entries from the shared library, since the deletion always happens only locally.

droidmonkey commented 5 years ago

If you are synchronizing or export then when you delete an entry it will save the new state to the share file. The recycle bin that is imported with the share is a lie. When you delete things from the share it moves that to your local recycle bin outside the share.

tideg commented 5 years ago

When you delete things from the share it moves that to your local recycle bin outside the share.

That is right, but it seems that deleted entries are not beeing synced! If you delete an entry on client A, it is no longer visible there (moved to the bin outside the share). But on Client B it is still visible in the share. Even if you add the share to a new client, the deleted entry still exists.

droidmonkey commented 5 years ago

I'll test that, certainly a bug if true. Also make sure the share file itself is syncing. Operations are not synced to the share file until you save the whole database where the operation was performed.

mstarke commented 5 years ago

The recycle bin in KeePass containers is just a group with benefits. The UI uses it to show special operations and the databases moves deleted items to it when the user has enabled the recycle bin.

Deleting an item in KeePass is a two-stage process if the recycle bin is enabled:

  1. Move item to the recycle bin
  2. Delete item from recycle bin (or empty the lot)

It's just step 2. if the recycle bin is disabled.

If you consider synchronising two databases (with the same recycle bin) it's easy to deal with both scenarios:

  1. Items moved to the recycle bin in the source database will get move to the recycle-bin in the destination database
  2. items deleted in the source database are also deleted in the destination database when synchronisation happens.

KeeShare cannot deal with scenario 1. if the recylce bin is not part of the shared groups. The algorithm creates the shared container with all items. That meand items moved to other locations aren't included in the shared container anymore. The entry that was moved to the recycle-bin isn't present in the synchronised container and there is no indication that it was moved to the recycle bin. The entry in the destination database is left untouched since KeeShare does not move items that aren't shared (anymore).

At the moment I have the following possible solutions:

  1. Mark items moved to the recycle bin to allow special treatment in KeeShare
  2. Always include the recycle bin in the synchronisation to ensure correct movement
  3. Directly delete entries without moving them to the recycle bin

Solution 3 seems to harsh, 1. introduces yet another special dataset so I would tend to work on 2. Thoughts?

droidmonkey commented 5 years ago

Maybe the answer is in my OP image, lol. KeeShare could create a recycle bin for an export/sync share within the share itself. I would add the share icon to the recycle bin to make it explicit perhaps? This would be option 4.

ckieschnick commented 5 years ago

Point 4 Is in my opinion confusing for the user on the client side since there are two types of recycle bins which behave differently (and in worst case a lot of them). Deleting a local entry will move the entry to the database recycle bin, importing a deleted entry would move the entry to the share recycle bin. This entry is than (at least with the current implementation) still shared and not seen as deleted for KeePassXC or any other client. Point 3 Seems harsh to me too.

Points 1 and 2 are somewhat unified in my mind as some kind of second uuid collection similar to the deleted objects. We can store the movement time and uuid and may move the shared entries to the recycle bin in the target database. Two problems remain:

mstarke commented 5 years ago

After some discussion with @ckieschnick we seem to agree on a solution that does not introduce a second recycle bin but rather uses TrashedObjects (comparable to Deleted Objects) to allow KeeShare to correctly reflect the users intend. This seems just like including the recycle bin into the database but has significant advantages. No direct access for the user to either see the data at all and to manipulate it in any way via the UI.

Using a simple group as trash introduces a lot of potential intentional and unintentional misuse. If the user opens the share database they see every item inside the trash which is a potential leak since we should include all items that are inside the exporting databases' recycle bin which might include items never intended for others. If using yet another custom data structure seems to far fetched an alternative could be to just include dummy entries inside the recycle bin - let's call them TrashedEntries This is however problematic since that allows for others to introduce side effects if the user that has the shared content shares something back and happens to actually delete such a TrashedEntry. This will result in unintended data loss in a database.

  1. Database A has Entry A which is never shared nor should it be shared
  2. Database A moves Entry A in the recyle bin
  3. Database A exports Group A via A.share which then includes TrashedEntryA
  4. Database B imports A.share
  5. Database B deletes TrashedEntryA
  6. Database B exports Group B via B.share
  7. Database A imports B.share
  8. Entry A in the recycle bin get's deleted since B.share contains it as DeletedObject

Additionally as described by @ckieschnick in his comment the introduction of even more recycle bins just adds to the already questionable problems arising with synchronising multiple databases with different recycle bins. That is, one wins, the other just keeps the looks but doesn't maintain the functionality

droidmonkey commented 5 years ago

I think we are in the realm of overthinking this problem. Before any time is spent coding, let me digest some of these options with some use cases.

tideg commented 5 years ago

Any news on this one?