-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Fix issues with reloading and handling of externally modified db file #10612
Fix issues with reloading and handling of externally modified db file #10612
Conversation
Oh this is exciting. Something I have been pondering for years how to fix properly. I haven't dived into your changes yet. |
I have wanted to introduce a new class called |
I can see how it would be cleaner to decouple those things. From my limited testing it has been working OK. Would be helpful if some users that run into this issue could help test and provide feedback. I haven't planned any further changes so, I will mark this PR ready for review. |
I think we leave this PR as-is and plan for a future change, we can work together on it. Big things I really really want to refactor towards:
For 2, this would mean you would build a "ChangeRequest" or similar that would then be passed to the Database object to enact that change. This solves a whole slew of problems with random parts of the code making deep changes to data structures in inconsistent ways. This also means we need to shift to const objects (groups and entries) everywhere except within the owning database object. |
ee05c2d
to
ea067f0
Compare
ea067f0
to
e1720c2
Compare
e1720c2
to
ed8ef94
Compare
- External modifications to the db file can no longer be missed. - Fixed dialogFinished signal of DatabaseOpenDialog was not emitted when dialog was closed via the 'X' (close) button - For reloading with a modified db, an additional choice has been added to allow the user to ignore the changes in the file on disk. - User is now presented with an unlock database dialog if reload fails to open the db automatically. For example when the user removed the YubiKey, failed to touch the YubiKey within the timeout period, or db pw has been changed. - Mark db as modified when db file is gone or invalid. Prevent saving when db is being reloaded Fixes keepassxreboot#5290
* Move to status bar updates instead of multiple message widget changes * If merge is triggered by a save action, continue on with the save action after the user makes their choice * Improve translation strings
ed8ef94
to
603ad15
Compare
@vuurvli3g I made several improvements, see the latest commit. This is ready to go, thank you again for solving this problem! |
…keepassxreboot#10612) Fixes keepassxreboot#5290 Fixes keepassxreboot#9062 Fixes keepassxreboot#8545 * Fix data loss on failed reload - External modifications to the db file can no longer be missed. - Fixed dialogFinished signal of DatabaseOpenDialog was not emitted when dialog was closed via the 'X' (close) button - For reloading with a modified db, an additional choice has been added to allow the user to ignore the changes in the file on disk. - User is now presented with an unlock database dialog if reload fails to open the db automatically. For example when the user removed the YubiKey, failed to touch the YubiKey within the timeout period, or db pw has been changed. - Mark db as modified when db file is gone or invalid. - Prevent saving when db is being reloaded - If merge is triggered by a save action, continue on with the save action after the user makes their choice --------- Co-authored-by: vuurvlieg <[email protected]> Co-authored-by: Jonathan White <[email protected]>
…#10612) Fixes #5290 Fixes #9062 Fixes #8545 * Fix data loss on failed reload - External modifications to the db file can no longer be missed. - Fixed dialogFinished signal of DatabaseOpenDialog was not emitted when dialog was closed via the 'X' (close) button - For reloading with a modified db, an additional choice has been added to allow the user to ignore the changes in the file on disk. - User is now presented with an unlock database dialog if reload fails to open the db automatically. For example when the user removed the YubiKey, failed to touch the YubiKey within the timeout period, or db pw has been changed. - Mark db as modified when db file is gone or invalid. - Prevent saving when db is being reloaded - If merge is triggered by a save action, continue on with the save action after the user makes their choice --------- Co-authored-by: vuurvlieg <[email protected]> Co-authored-by: Jonathan White <[email protected]>
Based on the discussion of issue #5290 and analyses, I've come to the following list of issues.
Some scenarios where a reload will not complete successfully.
There is code in place that is supposed to protect against this but it doesn't function as intended.
keepassxc/src/core/Database.cpp
Line 266 in 6f11422
keepassxc/src/core/FileWatcher.cpp
Line 107 in 6f11422
keepassxc/src/core/FileWatcher.cpp
Lines 119 to 125 in 6f11422
The current hash of the file is directly saved in
m_fileChecksum
upon detecting the change.The fail-safe check therefore only works as intended in the short window that the file was modified on disk but was not picked up by FileWatcher yet.
When keys of the db file have changed (password changed, YubiKey added/removed, ...). there should be a way to enter them.
This PR aims to address these issues.
Changes
In the case where the user has auto-reload disabled they will not see the "File changed - Want to reload?" dialog but can make a choice at the "Reload database" dialog (see screenshot).
Fixes #5290
Fixes #9062
Fixes #8545
Screenshots
Testing strategy
Added test-case to TestDatabase.
Manual testing.
Type of change