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

Recycle Bin is Imported using KeeShare #2798

Open
droidmonkey opened this issue Mar 18, 2019 · 12 comments
Open

Recycle Bin is Imported using KeeShare #2798

droidmonkey opened this issue Mar 18, 2019 · 12 comments

Comments

@droidmonkey
Copy link
Member

droidmonkey commented Mar 18, 2019

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
Copy link
Contributor

mstarke commented Mar 18, 2019

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
Copy link
Member Author

Yes agree completely

@tideg
Copy link

tideg commented Mar 25, 2019

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
Copy link
Member Author

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
Copy link

tideg commented Mar 26, 2019

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
Copy link
Member Author

droidmonkey commented Mar 26, 2019

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
Copy link
Contributor

mstarke commented Mar 26, 2019

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
Copy link
Member Author

droidmonkey commented Mar 26, 2019

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.

@droidmonkey droidmonkey modified the milestones: v2.4.1, v2.5.0 Apr 12, 2019
@ckieschnick
Copy link
Contributor

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:

  • How to handle those entries which were shared and moved out of the bin again. At this point, we need to reason when a entry was moved to the bin and when it was really deleted. If the entry was deleted from the bin in the source database after it was moved out of the bin in the importing database, we shouldn't delete the entry.
  • How to show the importing user that a former shared entry is no longer shared (moved out of the share folder). To handle this case, we would need to provide a list of non-shared entries which may lead to unwanted problems in scenarios in which a former synchronized or copied database is used with KeeShare.

@mstarke
Copy link
Contributor

mstarke commented Apr 16, 2019

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
Copy link
Member Author

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
Copy link

tideg commented May 29, 2019

Any news on this one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants