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& items) + DBusResult Collection::searchItems(const DBusClientPtr&, const StringStringMap& attributes, QList& 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(service(), QSet{this}, QSet{}); - 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& 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& unlocked, - QList& locked) const + QList& locked) { - QList colls; - auto ret = collections(colls); + // we can only search unlocked collections + QList 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 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& unlocked, - QList& locked) const; + QList& locked); Q_INVOKABLE DBusResult unlock(const DBusClientPtr& client, const QList& 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& unlocked) const; + private: FdoSecretsPlugin* m_plugin{nullptr}; QPointer m_databases{}; @@ -165,6 +173,7 @@ namespace FdoSecrets QList m_sessions{}; bool m_insideEnsureDefaultAlias{false}; + bool m_unlockingAnyDatabase{false}; // list of db currently has unlock dialog shown QHash m_unlockingDb{}; QSet 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 execNewDatabaseWizard(); void updateLastDatabases(const QString& filename); bool warnOnExport(); - void unlockAnyDatabaseInDialog(DatabaseOpenDialog::Intent intent); void displayUnlockDialog(); QPointer 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((m_dbData.size()))); + COMPARE(m_dbFile->write(m_dbData), static_cast(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(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(); @@ -1833,6 +1895,8 @@ bool TestGuiFdoSecrets::driveUnlockDialog() if (!dbOpenDlg->isVisible()) { return false; } + dbOpenDlg->setActiveDatabaseTab(target); + auto editPassword = dbOpenDlg->findChild("editPassword")->findChild("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);