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.

Sign up for GitHub

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

Correct multiple issues with Yubikey integration #880

Merged
merged 1 commit into from
Sep 23, 2017
+5 −3
Merged

Correct multiple issues with Yubikey integration #880

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/gui/DatabaseTabWidget.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,8 @@ bool DatabaseTabWidget::saveDatabase(Database* db)
emit messageDismissTab();
return true;
} else {
dbStruct.modified = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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);
+    }
+
 }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole saving structure needs to be redone. I suggest moving this improvement edge case to then.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@droidmonkey as you wish, but I thought the above diff was solving both problems.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this ready to be merged?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I will be refactoring the code above with my database pointer fixes and will address it there for 2.3

updateTabName(db);
emit messageTab(tr("Writing the database failed.").append("\n").append(errorMessage),
MessageWidget::Error);
return false;
Expand Down
4 changes: 2 additions & 2 deletions src/keys/YkChallengeResponseKey.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ QByteArray YkChallengeResponseKey::rawKey() const
*/
bool YkChallengeResponseKey::challenge(const QByteArray& challenge)
{
return this->challenge(challenge, 1);
return this->challenge(challenge, 2);
}

bool YkChallengeResponseKey::challenge(const QByteArray& challenge, unsigned retries)
Expand All @@ -70,8 +70,8 @@ bool YkChallengeResponseKey::challenge(const QByteArray& challenge, unsigned ret

QEventLoop loop;
QFutureWatcher<YubiKey::ChallengeResult> watcher;
watcher.setFuture(future);
connect(&watcher, SIGNAL(finished()), &loop, SLOT(quit()));
watcher.setFuture(future);
loop.exec();

if (m_blocking) {
Expand Down
2 changes: 1 addition & 1 deletion src/keys/drivers/YubiKey.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ YubiKey::ChallengeResult YubiKey::challenge(int slot, bool mayBlock, const QByte
QByteArray paddedChallenge = challenge;

// ensure that YubiKey::init() succeeded
if (m_yk == NULL) {
if (!init()) {
m_mutex.unlock();
return ERROR;
}
Expand Down