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

[PRE-RELEASE] Simultaneous editing doesn't work properly #3651

Closed
FlorentDotMe opened this issue Oct 22, 2019 · 7 comments
Closed

[PRE-RELEASE] Simultaneous editing doesn't work properly #3651

FlorentDotMe opened this issue Oct 22, 2019 · 7 comments

Comments

@FlorentDotMe
Copy link

Expected Behavior

When two or more users are working on a same keepassx database, creating, editing or deleting actions are not conflicting between users without being notified.

Current Behavior

Two users or more simultaneously editing an entry (same or different) will result on only the first to validate the modification will have it saved in the database.

Possible Solutions

  • Before saving modifications to an entry, make sure KeepassXC reloaded the database in background if it was modified.
  • Add a notification feature showing a message when multiple users are manipulating the same entry at the same time to avoid unexpected data override.

Steps to Reproduce

User A and User B are using KeepassXC on their own computer and are modifying the same keepassx database which is stored on a CIFS server.

  1. User A is editing the new entry E1.
  2. At the same time, User B is adding a new entry E2.
  3. User A is the first to validate his entry E1. Database is updated.
  4. Then User B validates his entry E2. Database is NOT updated.
  5. Finally, only E1 was added to the database and E2 is lost.

Context

We noticed this bug since PR #3612 allows us to have multiple users working on the same keepassx database. This bug was certainly present before the PR.

We only use Linux systems, so couldn't try to reproduce the bug on Windows systems.

Debug Info

KeePassXC - Version 2.5.0-snapshot
Build Type: Snapshot
Revision: 4cc06f9

Qt 5.11.3
Debugging mode is disabled.

Operating system: Debian GNU/Linux 10 (buster)
CPU architecture: x86_64
Kernel: linux 4.19.0-5-amd64

Enabled extensions:

  • Auto-Type

Cryptographic libraries:
libgcrypt 1.8.4

@phoerious
Copy link
Member

Stupid question, but do you have the setting to auto-reload the database on external changes on?

@FlorentDotMe
Copy link
Author

Stupid questions don't exist :) It just reveals I should have added more details.

Settings:

  • Safely save databases files
  • Backup database file before saving
  • Automatically save after every change
  • Don't mark database as modified for non-data changes
  • Automatically reload the database when modified externally

The database change detection is correctly working when users update the database one at a time.

@FlorentDotMe
Copy link
Author

Stupid questions don't exist :) It just reveals I should have added more details.

Settings:

  • Safely save databases files
  • Backup database file before saving
  • Automatically save after every change
  • Don't mark database as modified for non-data changes
  • Automatically reload the database when modified externally

The database change detection is correctly working when users update the database one at a time.

That rang a bell on my side. The "Automatically save after every change" setting leads to the situation I'm describing. Disabling it will just result on other complex situations, with inconsistency between users GUI Clients.

eg. If one user saves his modifications while another one didn't yet saved his own modifications, the Merge Request will popup. Once merged, both GUI with be inconsistent, and merged instance won't be savable anymore ...

@droidmonkey
Copy link
Member

droidmonkey commented Oct 22, 2019

When you say validate entry, that means pressing OK on the entry edit page, correct?

In other words, this only happens when the entry edit view is active?

What i don't understand is why the "check database file prior to save" is not kicking in. This was a major improvement put in by that PR.

@droidmonkey
Copy link
Member

For some context the way I tested this feature was to disable the various file notifications in the code one by one to make sure it's still worked at the end of the day.

@FlorentDotMe
Copy link
Author

FlorentDotMe commented Oct 22, 2019

The easier way to reproduce the behavior is to uncheck the Setting "Start only a single instance of KeePassXC" in order to have two KeePassXC instances on the same computer.

In following examples, I simulated parallel usage of the same keepassx database by using 2 local instances of KeepassXC. The behavior is the same than two KeepassXC on two computers with a network-shared database.

Even if the scenarios may seems complicated, they are likely to be faced in real conditions.

Scenario 1: Data loss

User A (left) and User B (right) are modifying the same keepassx database.

  1. Check Settings on both sides.
  2. User A adds the new entry E1 and saves the file. Modification is detected on User B.
  3. User A edits a new entry E2.
  4. At the same time, User B is editing a new entry E3.
  5. User A is the first to validate his entry E2 then saves the database.
  6. Then User B validates his entry E3.
  7. Finally, both users see E1 and E2 but E3 is lost.

keepassxc_debug_03

Update: the "Scenario 2: merging issue" previously described was likely due to a mistake by using an instance with 2.5.0-SNAPSHOT and the other with 2.4.3 !

@droidmonkey
Copy link
Member

Ok fantastic, I should be able to fix this today

droidmonkey added a commit that referenced this issue Oct 23, 2019
* Fix #3651

* Correct data loss when the database reloads due to a file change while creating a new entry. The issue occurred due to the "new parent group" pointer being invalid after the database is reloaded following merge.

* Also fix re-selecting entries following database file reload. If the entry was moved out of the current group it would result in an assert hit. This fix prevents recursively looking for the entry.
scoroi pushed a commit to scoroi/keepassxc that referenced this issue Nov 10, 2019
* Fix keepassxreboot#3651

* Correct data loss when the database reloads due to a file change while creating a new entry. The issue occurred due to the "new parent group" pointer being invalid after the database is reloaded following merge.

* Also fix re-selecting entries following database file reload. If the entry was moved out of the current group it would result in an assert hit. This fix prevents recursively looking for the entry.
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

3 participants