-
-
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
base: develop
Are you sure you want to change the base?
Fix issues with reloading and handling of externally modified db file #10612
Conversation
- 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. Fixes keepassxreboot#5290
Oh this is exciting. Something I have been pondering for years how to fix properly. I haven't dived into your changes yet. |
Mark db as modified when db file is gone or invalid.
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. |
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
Screenshots
Testing strategy
Added test-case to TestDatabase.
Manual testing.
Type of change