Remove lock file and cleanup file handling by droidmonkey · Pull Request #1231 · keepassxreboot/keepassxc

droidmonkey · 2017-11-26T23:38:03Z

Description

Closes #1002 by completely removing the lock file.

This PR also addresses several issues with file handling and cleans up the code to be more streamlined. One major change includes removing a prompt when locking the databases that would never occur with these corrections in place.

Motivation and context

First step to optimizing the saving of database files and also helps solve concurrent use issues reported.

How has this been tested?

Manually and with existing test cases. Future PR's will introduce testing with multiple clients open accessing the same database to ensure data integrity and accurate prompts.

Types of changes

✅ Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

✅ I have read the CONTRIBUTING document. [REQUIRED]
✅ My code follows the code style of this project. [REQUIRED]
✅ All new and existing tests passed. [REQUIRED]
✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]
❌ I have added tests to cover my changes.

phoerious · 2017-11-26T23:45:00Z

src/gui/DatabaseTabWidget.cpp

+            updateLastDatabases(dbStruct.fileInfo.absoluteFilePath());
            return true;
        } else {
            return false;


Move the return a level up and drop the else clause.

phoerious · 2017-11-26T23:47:43Z

src/gui/DatabaseTabWidget.cpp

-    updateLastDatabases(dbStruct.filePath);
+    updateLastDatabases(dbStruct.fileInfo.absoluteFilePath());

    if (!(pw.isNull() && keyFile.isEmpty())) {


Please reformulate this condition to !pw.isNull() || !keyFile.isEmpty(), It's a lot easier to comprehend.

phoerious · 2017-11-26T23:56:44Z

src/gui/DatabaseTabWidget.cpp


-    if (config()->get("AutoSaveAfterEveryChange").toBool() && dbStruct.saveToFilename) {
+    if (config()->get("AutoSaveAfterEveryChange").toBool() && !dbStruct.readOnly) {
        saveDatabase(db);


I would like an assertion here for dbWidget->currentMode() != DatabaseWidget::LockedMode (or perhaps even inside the saveDatabase() method?).

I put one inside saveDatabase() in place of the interlock.

Perhaps you should leave both. The assertion will ensure we notice in debug mode, but the safeguard condition will ensure it never happens in production either.

I was thinking that too. Will do!

phoerious · 2017-11-26T23:57:45Z

src/gui/DatabaseWidget.cpp

+                    .append("\n").append(file.errorString()),
            MessageWidget::Error);
+        // Mark db as modified since existing data may differ from file or file was deleted
+        m_db->modified();


This should be called setModified().

This directly calls the modified signal so that both the tab widget and dbwidget get updated as "modified". There is no other way to do that in the current architecture. I will add a //hack note.

droidmonkey · 2017-11-27T03:51:24Z

Made the requested changes. You'll notice that the saveDatabaseAs function shrunk. I found that a lot of the work was occurring already in saveDatabase function so I streamlined further.

droidmonkey · 2017-12-04T04:09:04Z

This is now ready for merge.

louib · 2017-12-19T23:45:03Z

src/gui/DatabaseTabWidget.cpp

-            dbStruct.lockFile = lockFile.take();
-            updateTabName(db);
-            updateLastDatabases(dbStruct.filePath);
+            dbStruct.dbWidget->updateFilename(dbStruct.fileInfo.absoluteFilePath());


@droidmonkey maybe we can rename this updateFilePath?

Absolutely, fixed in other places as well.

louib · 2017-12-19T23:45:18Z

src/gui/DatabaseTabWidget.cpp

-        QString fileName = fileDialog()->getSaveFileName(this, tr("Save database as"),
-                                                        oldFileName, tr("KeePass 2 Database").append(" (*.kdbx)"),
+        QString newFilePath = fileDialog()->getSaveFileName(this, tr("Save database as"),
+                                                        oldFilePath, tr("KeePass 2 Database").append(" (*.kdbx)"),


@droidmonkey these arguments need to be realigned.

louib · 2017-12-19T23:49:54Z

src/gui/DatabaseTabWidget.cpp

-    if (dbStruct.saveToFilename || dbStruct.readOnly) {
+    if (dbStruct.fileInfo.exists()) {
        if (db->metadata()->name().isEmpty()) {
-            tabName = dbStruct.fileName;


@droidmonkey was there a possibility that the database was readOnly and the fileName was not defined?

I am not sure what you mean by this. readOnly is only defined (with these new fixes) when the database file cannot be opened using QIODevice::ReadWrite and it can be opened using QIODevice::ReadOnly (see Line 152).

@droidmonkey nevermind, I was thinking about a possible execution path that might have occurred with the previous code.

louib · 2017-12-19T23:52:04Z

src/gui/DatabaseTabWidget.cpp

-        DatabaseWidget::Mode mode = dbWidget->currentMode();
-
-        if ((mode != DatabaseWidget::ViewMode && mode != DatabaseWidget::EditMode)
-                || !dbWidget->dbHasKey()) {


@droidmonkey shouldn't we keep the dbHasKey() check to prevent locking?

Added back in, good idea.

louib

Tested locally, looks like it's working fine when modifying the database from 2 different locations.

TheZ3ro · 2017-12-20T23:11:44Z

src/gui/DatabaseTabWidget.cpp

+    // Never allow saving a locked database; it causes corruption
+    Q_ASSERT(dbStruct.dbWidget->currentMode() != DatabaseWidget::LockedMode);
+    // Release build interlock
    if (dbStruct.dbWidget->currentMode() == DatabaseWidget::LockedMode) {


This if won't be reached if true since the above assert will abort the execution. Better remove one of them

EDIT: Nevermind, the assert will only be executed in debug mode.

louib · 2017-12-21T14:17:25Z

@droidmonkey were you planning to add some tests for that PR?

droidmonkey · 2017-12-22T04:33:02Z

I was going to add tests for "concurrent" access of the same file in phase 2 of these changes. Phase 2 is refactoring the saving process entirely to make it asynchronous and robust to file sync services.

phoerious · 2017-12-27T19:07:49Z

Are you going to add more changes or can this be merged? Tests maybe?

droidmonkey · 2017-12-28T01:09:34Z

Its ready for merging.

phoerious · 2017-12-28T07:40:28Z

OK, please rebase then.

TheZ3ro · 2018-01-04T22:44:10Z

@droidmonkey can you rebase please?

- Add support for KDBX 4.0, Argon2 and ChaCha20 [#148, #1179, #1230, #1494] - Add SSH Agent feature [#1098, #1450, #1463] - Add preview panel with details of the selected entry [#879, #1338] - Add more and configurable columns to entry table and allow copying of values by double click [#1305] - Add KeePassXC-Browser API as a replacement for KeePassHTTP [#608] - Deprecate KeePassHTTP [#1392] - Add support for Steam one-time passwords [#1206] - Add support for multiple Auto-Type sequences for a single entry [#1390] - Adjust YubiKey HMAC-SHA1 challenge-response key generation for KDBX 4.0 [#1060] - Replace qHttp with cURL for website icon downloads [#1460] - Remove lock file [#1231] - Add option to create backup file before saving [#1385] - Ask to save a generated password before closing the entry password generator [#1499] - Resolve placeholders recursively [#1078] - Add Auto-Type button to the toolbar [#1056] - Improve window focus handling for Auto-Type dialogs [#1204, #1490] - Auto-Type dialog and password generator can now be exited with ESC [#1252, #1412] - Add optional dark tray icon [#1154] - Add new "Unsafe saving" option to work around saving problems with file sync services [#1385] - Add IBus support to AppImage and additional image formats to Windows builds [#1534, #1537] - Add diceware password generator to CLI [#1406] - Add --key-file option to CLI [#816, #824] - Add DBus interface for opening and closing KeePassXC databases [#283] - Add KDBX compression options to database settings [#1419] - Discourage use of old fixed-length key files in favor of arbitrary files [#1326, #1327] - Correct reference resolution in entry fields [#1486] - Fix window state and recent databases not being remembered on exit [#1453] - Correct history item generation when configuring TOTP for an entry [#1446] - Correct multiple TOTP bugs [#1414] - Automatic saving after every change is now a default [#279] - Allow creation of new entries during search [#1398] - Correct menu issues on macOS [#1335] - Allow compilation on OpenBSD [#1328] - Improve entry attachments view [#1139, #1298] - Fix auto lock for Gnome and Xfce [#910, #1249] - Don't remember key files in file dialogs when the setting is disabled [#1188] - Improve database merging and conflict resolution [#807, #1165] - Fix macOS pasteboard issues [#1202] - Improve startup times on some platforms [#1205] - Hide the notes field by default [#1124] - Toggle main window by clicking tray icon with the middle mouse button [#992] - Fix custom icons not copied over when databases are merged [#1008] - Allow use of DEL key to delete entries [#914] - Correct intermittent crash due to stale history items [#1527] - Sanitize newline characters in title, username and URL fields [#1502] - Reopen previously opened databases in correct order [#774] - Use system's zxcvbn library if available [#701] - Implement various i18n improvements [#690, #875, #1436]

droidmonkey added this to the v2.3.0 milestone Nov 26, 2017

droidmonkey requested review from TheZ3ro and phoerious November 26, 2017 23:38

droidmonkey force-pushed the refactor/remove-lockfile branch from 0490305 to 0afa287 Compare November 26, 2017 23:40

phoerious requested changes Nov 27, 2017

View reviewed changes

droidmonkey force-pushed the refactor/remove-lockfile branch from 6867ef2 to d3153e1 Compare November 27, 2017 03:31

droidmonkey force-pushed the refactor/remove-lockfile branch 2 times, most recently from 585995f to 8385dbd Compare December 4, 2017 04:08

droidmonkey force-pushed the refactor/remove-lockfile branch from 8385dbd to 7e50be2 Compare December 8, 2017 03:13

louib reviewed Dec 19, 2017

View reviewed changes

droidmonkey requested review from louib and removed request for TheZ3ro December 20, 2017 15:47

phoerious approved these changes Dec 20, 2017

View reviewed changes

louib mentioned this pull request Dec 20, 2017

Add \n in merge request message. #1310

Merged

louib approved these changes Dec 20, 2017

View reviewed changes

TheZ3ro reviewed Dec 20, 2017

View reviewed changes

phoerious added the core label Dec 21, 2017

droidmonkey force-pushed the refactor/remove-lockfile branch 2 times, most recently from 6209d13 to 1b09b7a Compare December 28, 2017 22:55

droidmonkey force-pushed the refactor/remove-lockfile branch from 1b09b7a to 0052755 Compare January 4, 2018 23:57

droidmonkey added 4 commits January 4, 2018 19:09

Remove lock file and cleanup file handling [#1002]

c2d0449

Addressed comments; further streamlined code

a80d1bb

Corrected use of canonicalfile info in autoopen code

36e5377

Consistency using filepath and minor code fixes

1a7b874

droidmonkey force-pushed the refactor/remove-lockfile branch from 0052755 to 1a7b874 Compare January 5, 2018 00:09

droidmonkey merged commit 8f99764 into develop Jan 5, 2018

droidmonkey deleted the refactor/remove-lockfile branch January 5, 2018 01:55

This was referenced Jan 10, 2018

Lock file made before database even opened #1372

Closed

Database already opened #1240

Closed

Algebro7 mentioned this pull request Mar 7, 2018

"Permission denied" when trying to open read only password database #1187

Closed

Uh oh!

Conversation

droidmonkey commented Nov 26, 2017

Description

Motivation and context

How has this been tested?

Types of changes

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phoerious Nov 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phoerious Nov 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

droidmonkey commented Nov 27, 2017

Uh oh!

droidmonkey commented Dec 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

louib left a comment

Choose a reason for hiding this comment

Uh oh!

TheZ3ro Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

louib commented Dec 21, 2017

Uh oh!

droidmonkey commented Dec 22, 2017

Uh oh!

phoerious commented Dec 27, 2017

Uh oh!

droidmonkey commented Dec 28, 2017

Uh oh!

phoerious commented Dec 28, 2017

Uh oh!

TheZ3ro commented Jan 4, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

phoerious Nov 26, 2017 •

edited

Loading

phoerious Nov 26, 2017 •

edited

Loading

TheZ3ro Dec 20, 2017 •

edited

Loading