diff options
author | Aetf <aetf@unlimited-code.works> | 2022-05-07 10:38:10 +0300 |
---|---|---|
committer | Jonathan White <support@dmapps.us> | 2022-06-27 18:01:06 +0300 |
commit | 412552aa6efb5a6ae26bce63edb857f47ee89de6 (patch) | |
tree | 6a48c27911d10c8d175dbd44600ebe74ef0e2e28 | |
parent | 1537604d9b009a4250d6131830bdd8454abf5409 (diff) |
FdoSecrets: add smarter handling of database unlock requests
This commit implements the following logic:
* If there're already unlocked collections, just use those,
* otherwise, show the unlock dialog until there's an unlocked and exposed collection.
* Fixes #7574
-rw-r--r-- | src/fdosecrets/objects/Collection.cpp | 21 | ||||
-rw-r--r-- | src/fdosecrets/objects/Service.cpp | 76 | ||||
-rw-r--r-- | src/fdosecrets/objects/Service.h | 11 | ||||
-rw-r--r-- | src/gui/DatabaseTabWidget.h | 2 | ||||
-rw-r--r-- | tests/gui/TestGuiFdoSecrets.cpp | 70 | ||||
-rw-r--r-- | tests/gui/TestGuiFdoSecrets.h | 3 |
6 files changed, 151 insertions, 32 deletions
diff --git a/src/fdosecrets/objects/Collection.cpp b/src/fdosecrets/objects/Collection.cpp index 68ccc9dcc..4cc6ca537 100644 --- a/src/fdosecrets/objects/Collection.cpp +++ b/src/fdosecrets/objects/Collection.cpp @@ -210,8 +210,7 @@ namespace FdoSecrets return {}; } - DBusResult - Collection::searchItems(const DBusClientPtr& client, const StringStringMap& attributes, QList<Item*>& items) + DBusResult Collection::searchItems(const DBusClientPtr&, const StringStringMap& attributes, QList<Item*>& items) { items.clear(); @@ -220,24 +219,6 @@ namespace FdoSecrets return ret; } - if (backendLocked() && settings()->unlockBeforeSearch()) { - // do a blocking unlock prompt first. - // while technically not correct, this should improve compatibility. - // see issue #4443 - auto prompt = PromptBase::Create<UnlockPrompt>(service(), QSet<Collection*>{this}, QSet<Item*>{}); - if (!prompt) { - return QDBusError::InternalError; - } - // we don't know the windowId to show the prompt correctly, - // but the default of showing over the KPXC main window should be good enough - ret = prompt->prompt(client, ""); - // blocking wait - QEventLoop loop; - connect(prompt, &PromptBase::completed, &loop, &QEventLoop::quit); - loop.exec(); - } - - // check again because the prompt may be cancelled if (backendLocked()) { // searchItems should work, whether `this` is locked or not. // however, we can't search items the same way as in gnome-keying, diff --git a/src/fdosecrets/objects/Service.cpp b/src/fdosecrets/objects/Service.cpp index d9b54710c..ae1e9d4b6 100644 --- a/src/fdosecrets/objects/Service.cpp +++ b/src/fdosecrets/objects/Service.cpp @@ -184,6 +184,30 @@ namespace FdoSecrets return {}; } + DBusResult Service::unlockedCollections(QList<Collection*>& unlocked) const + { + auto ret = collections(unlocked); + if (ret.err()) { + return ret; + } + + // filter out locked collections + auto it = unlocked.begin(); + while (it != unlocked.end()) { + bool isLocked = true; + ret = (*it)->locked(isLocked); + if (ret.err()) { + return ret; + } + if (isLocked) { + it = unlocked.erase(it); + } else { + ++it; + } + } + return {}; + } + DBusResult Service::openSession(const DBusClientPtr& client, const QString& algorithm, const QVariant& input, @@ -242,15 +266,43 @@ namespace FdoSecrets DBusResult Service::searchItems(const DBusClientPtr& client, const StringStringMap& attributes, QList<Item*>& unlocked, - QList<Item*>& locked) const + QList<Item*>& locked) { - QList<Collection*> colls; - auto ret = collections(colls); + // we can only search unlocked collections + QList<Collection*> unlockedColls; + auto ret = unlockedCollections(unlockedColls); if (ret.err()) { return ret; } - for (const auto& coll : asConst(colls)) { + while (unlockedColls.isEmpty() && settings()->unlockBeforeSearch()) { + // enable compatibility mode by making sure at least one database is unlocked + QEventLoop loop; + bool wasAccepted = false; + connect(this, &Service::doneUnlockDatabaseInDialog, &loop, [&](bool accepted) { + wasAccepted = accepted; + loop.quit(); + }); + + doUnlockAnyDatabaseInDialog(); + + // blocking wait + loop.exec(); + + if (!wasAccepted) { + // user cancelled, do not proceed + qWarning() << "user cancelled"; + return {}; + } + + // need to recompute this because collections may disappear while in event loop + ret = unlockedCollections(unlockedColls); + if (ret.err()) { + return ret; + } + } + + for (const auto& coll : asConst(unlockedColls)) { QList<Item*> items; ret = coll->searchItems(client, attributes, items); if (ret.err()) { @@ -524,7 +576,7 @@ namespace FdoSecrets } // check if the db is already being unlocked to prevent multiple dialogs for the same db - if (m_unlockingDb.contains(dbWidget)) { + if (m_unlockingAnyDatabase || m_unlockingDb.contains(dbWidget)) { return; } @@ -536,21 +588,33 @@ namespace FdoSecrets m_databases->unlockDatabaseInDialog(dbWidget, DatabaseOpenDialog::Intent::None); } + void Service::doUnlockAnyDatabaseInDialog() + { + if (m_unlockingAnyDatabase || !m_unlockingDb.isEmpty()) { + return; + } + m_unlockingAnyDatabase = true; + + m_databases->unlockAnyDatabaseInDialog(DatabaseOpenDialog::Intent::None); + } + void Service::onDatabaseUnlockDialogFinished(bool accepted, DatabaseWidget* dbWidget) { - if (!m_unlockingDb.contains(dbWidget)) { + if (!m_unlockingAnyDatabase && !m_unlockingDb.contains(dbWidget)) { // not our concern return; } if (!accepted) { emit doneUnlockDatabaseInDialog(false, dbWidget); + m_unlockingAnyDatabase = false; m_unlockingDb.remove(dbWidget); } else { // delay the done signal to when the database is actually done with unlocking // this is a oneshot connection to prevent superfluous signals auto conn = connect(dbWidget, &DatabaseWidget::databaseUnlocked, this, [dbWidget, this]() { emit doneUnlockDatabaseInDialog(true, dbWidget); + m_unlockingAnyDatabase = false; disconnect(m_unlockingDb.take(dbWidget)); }); m_unlockingDb[dbWidget] = conn; diff --git a/src/fdosecrets/objects/Service.h b/src/fdosecrets/objects/Service.h index 022023cae..5ec7499b1 100644 --- a/src/fdosecrets/objects/Service.h +++ b/src/fdosecrets/objects/Service.h @@ -65,7 +65,7 @@ namespace FdoSecrets Q_INVOKABLE DBusResult searchItems(const DBusClientPtr& client, const StringStringMap& attributes, QList<Item*>& unlocked, - QList<Item*>& locked) const; + QList<Item*>& locked); Q_INVOKABLE DBusResult unlock(const DBusClientPtr& client, const QList<DBusObject*>& objects, @@ -125,6 +125,12 @@ namespace FdoSecrets */ void doUnlockDatabaseInDialog(DatabaseWidget* dbWidget); + /** + * Async, connect to signal doneUnlockDatabaseInDialog for finish notification + * @param dbWidget + */ + void doUnlockAnyDatabaseInDialog(); + private slots: void ensureDefaultAlias(); @@ -154,6 +160,8 @@ namespace FdoSecrets */ Collection* findCollection(const DatabaseWidget* db) const; + DBusResult unlockedCollections(QList<Collection*>& unlocked) const; + private: FdoSecretsPlugin* m_plugin{nullptr}; QPointer<DatabaseTabWidget> m_databases{}; @@ -165,6 +173,7 @@ namespace FdoSecrets QList<Session*> m_sessions{}; bool m_insideEnsureDefaultAlias{false}; + bool m_unlockingAnyDatabase{false}; // list of db currently has unlock dialog shown QHash<const DatabaseWidget*, QMetaObject::Connection> m_unlockingDb{}; QSet<const DatabaseWidget*> m_lockingDb{}; // list of db being locking diff --git a/src/gui/DatabaseTabWidget.h b/src/gui/DatabaseTabWidget.h index 8432116ed..e823043a6 100644 --- a/src/gui/DatabaseTabWidget.h +++ b/src/gui/DatabaseTabWidget.h @@ -75,6 +75,7 @@ public slots: void closeDatabaseFromSender(); void unlockDatabaseInDialog(DatabaseWidget* dbWidget, DatabaseOpenDialog::Intent intent); void unlockDatabaseInDialog(DatabaseWidget* dbWidget, DatabaseOpenDialog::Intent intent, const QString& filePath); + void unlockAnyDatabaseInDialog(DatabaseOpenDialog::Intent intent); void relockPendingDatabase(); void showDatabaseSecurity(); @@ -106,7 +107,6 @@ private: QSharedPointer<Database> execNewDatabaseWizard(); void updateLastDatabases(const QString& filename); bool warnOnExport(); - void unlockAnyDatabaseInDialog(DatabaseOpenDialog::Intent intent); void displayUnlockDialog(); QPointer<DatabaseWidgetStateSync> m_dbWidgetStateSync; diff --git a/tests/gui/TestGuiFdoSecrets.cpp b/tests/gui/TestGuiFdoSecrets.cpp index 656ea135f..0f9d62040 100644 --- a/tests/gui/TestGuiFdoSecrets.cpp +++ b/tests/gui/TestGuiFdoSecrets.cpp @@ -183,7 +183,7 @@ void TestGuiFdoSecrets::init() m_dbFile.reset(new TemporaryFile()); // Write the temp storage to a temp database file for use in our tests VERIFY(m_dbFile->open()); - COMPARE(m_dbFile->write(m_dbData), static_cast<qint64>((m_dbData.size()))); + COMPARE(m_dbFile->write(m_dbData), static_cast<qint64>(m_dbData.size())); m_dbFile->close(); // make sure window is activated or focus tests may fail @@ -198,6 +198,12 @@ void TestGuiFdoSecrets::init() // by default expose the root group FdoSecrets::settings()->setExposedGroup(m_db, m_db->rootGroup()->uuid()); VERIFY(m_dbWidget->save()); + + // enforce consistent default settings at the beginning + FdoSecrets::settings()->setUnlockBeforeSearch(false); + FdoSecrets::settings()->setShowNotification(false); + FdoSecrets::settings()->setConfirmAccessItem(false); + FdoSecrets::settings()->setEnabled(false); } // Every test ends with closing the temp database without saving @@ -388,6 +394,62 @@ void TestGuiFdoSecrets::testServiceSearchBlockingUnlock() } } +void TestGuiFdoSecrets::testServiceSearchBlockingUnlockMultiple() +{ + // setup: two databases, both locked, one with exposed db, the other not. + + // add another database tab with a database with no exposed group + // to avoid modify the original, copy to a temp file first + QFile sourceDbFile(QStringLiteral(KEEPASSX_TEST_DATA_DIR "/NewDatabase2.kdbx")); + QByteArray dbData; + VERIFY(sourceDbFile.open(QIODevice::ReadOnly)); + VERIFY(Tools::readAllFromDevice(&sourceDbFile, dbData)); + sourceDbFile.close(); + + QTemporaryFile anotherFile; + VERIFY(anotherFile.open()); + COMPARE(anotherFile.write(dbData), static_cast<qint64>(dbData.size())); + anotherFile.close(); + + m_tabWidget->addDatabaseTab(anotherFile.fileName(), false); + auto anotherWidget = m_tabWidget->currentDatabaseWidget(); + + auto service = enableService(); + VERIFY(service); + + // when there are multiple locked databases, + // repeatly show the dialog until there is at least one unlocked collection + FdoSecrets::settings()->setUnlockBeforeSearch(true); + + // when only unlocking the one with no exposed group, a second dialog is shown + lockDatabaseInBackend(); + { + bool unlockDialogWorks = false; + QTimer::singleShot(50, [&]() { + unlockDialogWorks = driveUnlockDialog(anotherWidget); + QTimer::singleShot(50, [&]() { unlockDialogWorks &= driveUnlockDialog(); }); + }); + + DBUS_GET2(unlocked, locked, service->SearchItems({{"Title", "Sample Entry"}})); + VERIFY(unlockDialogWorks); + COMPARE(locked, {}); + COMPARE(unlocked.size(), 1); + } + + // when unlocking the one with exposed group, the other one remains locked + lockDatabaseInBackend(); + { + bool unlockDialogWorks = false; + QTimer::singleShot(50, [&]() { unlockDialogWorks = driveUnlockDialog(m_dbWidget); }); + + DBUS_GET2(unlocked, locked, service->SearchItems({{"Title", "Sample Entry"}})); + VERIFY(unlockDialogWorks); + COMPARE(locked, {}); + COMPARE(unlocked.size(), 1); + VERIFY(anotherWidget->isLocked()); + } +} + void TestGuiFdoSecrets::testServiceSearchForce() { auto service = enableService(); @@ -1625,7 +1687,7 @@ void TestGuiFdoSecrets::testNoExposeRecycleBin() void TestGuiFdoSecrets::lockDatabaseInBackend() { - m_dbWidget->lock(); + m_tabWidget->lockDatabases(); m_db.reset(); processEvents(); } @@ -1825,7 +1887,7 @@ bool TestGuiFdoSecrets::driveNewDatabaseWizard() return ret; } -bool TestGuiFdoSecrets::driveUnlockDialog() +bool TestGuiFdoSecrets::driveUnlockDialog(DatabaseWidget* target) { processEvents(); auto dbOpenDlg = m_tabWidget->findChild<DatabaseOpenDialog*>(); @@ -1833,6 +1895,8 @@ bool TestGuiFdoSecrets::driveUnlockDialog() if (!dbOpenDlg->isVisible()) { return false; } + dbOpenDlg->setActiveDatabaseTab(target); + auto editPassword = dbOpenDlg->findChild<PasswordWidget*>("editPassword")->findChild<QLineEdit*>("passwordEdit"); VERIFY(editPassword); editPassword->setFocus(); diff --git a/tests/gui/TestGuiFdoSecrets.h b/tests/gui/TestGuiFdoSecrets.h index 9a43b78a3..1624eed49 100644 --- a/tests/gui/TestGuiFdoSecrets.h +++ b/tests/gui/TestGuiFdoSecrets.h @@ -67,6 +67,7 @@ private slots: void testServiceEnableNoExposedDatabase(); void testServiceSearch(); void testServiceSearchBlockingUnlock(); + void testServiceSearchBlockingUnlockMultiple(); void testServiceSearchForce(); void testServiceUnlock(); void testServiceUnlockDatabaseConcurrent(); @@ -104,7 +105,7 @@ private slots: void testDuplicateName(); private: - bool driveUnlockDialog(); + bool driveUnlockDialog(DatabaseWidget* target = nullptr); bool driveNewDatabaseWizard(); bool driveAccessControlDialog(bool remember = true, bool includeFutureEntries = false); bool waitForSignal(QSignalSpy& spy, int expectedCount); |