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

Fix issues with reloading and handling of externally modified db file #10612

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

vuurvli3g
Copy link

@vuurvli3g vuurvli3g commented Apr 21, 2024

Based on the discussion of issue #5290 and analyses, I've come to the following list of issues.

  • Reloading is one-shot. If it is not completed successfully there is no mechanism to retry:
    Some scenarios where a reload will not complete successfully.
    • If the user is busy editing a group/entry the reload is ignored.
    • The user has auto-reload disabled and chooses "No" upon being asked to load the external changes.
    • The db file could not be opened with the same key used to open the db:
      • The YubiKey is not inserted
      • A different YubiKey is inserted (programmed with the same secret)
      • The window for touching the YubiKey was missed
      • The keys of the db file have changed (password changed, YubiKey added/removed, ...)
  • Overwriting a db file that was modified externally should not be possible (unless explicitly requested):
    There is code in place that is supposed to protect against this but it doesn't function as intended.
    if (!m_fileWatcher->hasSameFileChecksum()) {

    return calculateChecksum() == m_fileChecksum;

    AsyncTask::runThenCallback([=] { return calculateChecksum(); },
    this,
    [=](QByteArray checksum) {
    if (checksum != m_fileChecksum) {
    m_fileChecksum = checksum;
    m_fileChangeDelayTimer.start(0);
    }

    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.
  • There is no way to provide other keys for reloading a db file:
    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

  • Cannot overwrite an externally modified db file unless requested.
  • Trying to save (overwrite) an externally modified db file will trigger a reload, serving as retry mechanism.
    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).
  • An additional choice is added to the previously known "Merge Request" dialog, now "Reload database" dialog (see screenshot), to allow the user to ignore the changes in the file on disk (and overwrite the file).
  • If during the reload the db could not be opened with the original key (due to missing YubiKey etc..) the user is presented with the "Unlock Database" dialog (see screenshot).

Fixes #5290

Screenshots

reload_db_unsaved_changes

reload_db_unlock_dlg

Testing strategy

Added test-case to TestDatabase.
Manual testing.

Type of change

  • ✅ Bug fix (non-breaking change that fixes an issue)
  • ✅ New feature (change that adds functionality)

vuurvlieg added 5 commits April 19, 2024 17:24
- 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
@droidmonkey
Copy link
Member

Oh this is exciting. Something I have been pondering for years how to fix properly. I haven't dived into your changes yet.

@droidmonkey
Copy link
Member

droidmonkey commented Apr 28, 2024

I have wanted to introduce a new class called DatabaseFileManager or similar that would take all the file handling functions (Open, Save, Reload, etc) away from Database and DatabaseWidget. This is important because Database should just be the in-memory representation of the database and be the sole owner of the RootGroup and all it's children. Ripping out the file management code would make this relationship much cleaner. We also want to avoid the current code that splits file handling functions between the presentation layer (DatabaseWidget) and the data layer (Database).

@vuurvli3g
Copy link
Author

I can see how it would be cleaner to decouple those things.
Is there something in relation to this PR you would like me to do?

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.
The main that has come forward for myself was that It might be nicer to use a different color for the "Reloading database..." message widget. When there is a YubiKey involved the amount of blue bars on the UI can be a bit much.
Perhaps making it the warning color (yellow) would make for a better visual experience.

I haven't planned any further changes so, I will mark this PR ready for review.

@vuurvli3g vuurvli3g marked this pull request as ready for review May 9, 2024 13:43
@droidmonkey
Copy link
Member

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:

  1. The aforementioned DatabaseFileManager
  2. Transactional changes to entries, groups, and database
  3. Implement Undo/Redo and Change Tracking
  4. Move away from "resolveMultiplePlaceholders" garbage and introduce smart caching of "display data" outside of "stored data"

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.

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

Successfully merging this pull request may close these issues.

Data loss when database file changes without Yubikey available
2 participants