Correct multiple issues with Yubikey integration #880
Conversation
|
Hello, i have tested this pull and all work's fine for me. I will do some more tests. |
|
Hi, What is the time-frame for the release that this pull request is targeting? The bug that this pull request fixes is particularly severe in that it can cause database corruption and interrupts database saving. Thanks, |
|
Target milestone is 2.2.1. |
|
Basically I'd want to release shortly after this is merged. |
|
@keepassxreboot/core-developers ready to merge this? |
| @@ -363,6 +363,8 @@ bool DatabaseTabWidget::saveDatabase(Database* db) | |||
| emit messageDismissTab(); | |||
| return true; | |||
| } else { | |||
| dbStruct.modified = true; | |||
There was a problem hiding this comment.
@droidmonkey the only problem I have with that is that if the database file was not modified but the user tried to save manually, and the save fails, the db will be marked as dirty even though no changes have been made. You can try for example by removing write permission to the db file after opening it in keepassxc. Of course you could argue that it's not gonna happen often.
What about marking the database dirty before calling autosave in DatabaseTabWidget::modified():
diff --git a/src/gui/DatabaseTabWidget.cpp b/src/gui/DatabaseTabWidget.cpp
--- a/src/gui/DatabaseTabWidget.cpp
+++ b/src/gui/DatabaseTabWidget.cpp
@@ -780,16 +780,17 @@ void DatabaseTabWidget::modified()
Database* db = static_cast<Database*>(sender());
DatabaseManagerStruct& dbStruct = m_dbList[db];
- if (config()->get("AutoSaveAfterEveryChange").toBool() && dbStruct.saveToFilename) {
- saveDatabase(db);
- return;
- }
-
if (!dbStruct.modified) {
dbStruct.modified = true;
dbStruct.dbWidget->databaseModified();
updateTabName(db);
}
+
+ if (config()->get("AutoSaveAfterEveryChange").toBool() && dbStruct.saveToFilename) {
+ saveDatabase(db);
+ }
+
}
There was a problem hiding this comment.
The whole saving structure needs to be redone. I suggest moving this improvement edge case to then.
There was a problem hiding this comment.
@droidmonkey as you wish, but I thought the above diff was solving both problems.
There was a problem hiding this comment.
I will need to think about that a little bit. The case I am trying to solve with my fix is the user requested a save and it failed to save so I am marking the database as dirty because I do not know if the state can be trusted. This incidentally fixes the issue when an auto save fails and never marks the database dirty (ie. the database != save file).
There was a problem hiding this comment.
Is this ready to be merged?
There was a problem hiding this comment.
Yes. I will be refactoring the code above with my database pointer fixes and will address it there for 2.3
* Fixed database not showing modified after failed save * Fixed Yubikey not being redetected after replug * Fixed single shot challenge resulting in failed saves
droidmonkey
force-pushed
the
hotfix/robust-yubikey
branch
from
dfbf50d
to
c9b7ba6
Compare
- Corrected multiple snap issues [#934, #1011] - Corrected multiple custom icon issues [#708, #719, #994] - Corrected multiple Yubikey issues [#880] - Fixed single instance preventing load on occasion [#997] - Keep entry history when merging databases [#970] - Prevent data loss if passwords were mismatched [#1007] - Fixed crash after merge [#941] - Added configurable auto-type default delay [#703] - Unlock database dialog window comes to front [#663] - Translation and compiling fixes
Description
How has this been tested?
Manually using a Yubikey Neo
Types of changes
Checklist:
-DWITH_ASAN=ON. [REQUIRED]