From a31c5ba006198f2fdadfd6c4f587cfed602c81c6 Mon Sep 17 00:00:00 2001 From: Aetf Date: Sat, 18 Sep 2021 19:55:37 -0400 Subject: [PATCH] FdoSecrets: Implement unlock before search Fixes #6942 and fixes #4443 - Return number of deleted entries - Fix minor memory leak - FdoSecrets: make all prompt truly async per spec and update tests * the waited signal may already be emitted before calling spy.wait(), causing the test to fail. This commit checks the count before waiting. * check unlock result after waiting for signal - FdoSecrets: implement unlockBeforeSearch option - FdoSecrets: make search always work regardless of entry group searching settings, fixes #6942 - FdoSecrets: cleanup gracefully even if some test failed - FdoSecrets: make it safe to call prompts concurrently - FdoSecrets: make sure in unit test we click on the correct dialog Note on the unit tests: objects are not deleted (due to deleteLater event not handled). So there may be multiple AccessControlDialog. But only one of it is visible and is the correctly one to click on. Before this change, a random one may be clicked on, causing the completed signal never be sent. --- share/translations/keepassxc_en.ts | 20 +- src/core/Config.cpp | 1 + src/core/Config.h | 1 + src/fdosecrets/FdoSecretsSettings.cpp | 10 + src/fdosecrets/FdoSecretsSettings.h | 3 + src/fdosecrets/dbus/DBusMgr.cpp | 2 - src/fdosecrets/dbus/DBusObject.h | 2 +- src/fdosecrets/objects/Collection.cpp | 115 +++-- src/fdosecrets/objects/Collection.h | 24 +- src/fdosecrets/objects/Item.cpp | 9 +- src/fdosecrets/objects/Item.h | 7 +- src/fdosecrets/objects/Prompt.cpp | 334 ++++++------ src/fdosecrets/objects/Prompt.h | 120 +++-- src/fdosecrets/objects/Service.cpp | 54 +- src/fdosecrets/objects/Service.h | 23 +- .../widgets/SettingsWidgetFdoSecrets.cpp | 2 + .../widgets/SettingsWidgetFdoSecrets.ui | 23 +- src/gui/GuiTools.cpp | 5 +- src/gui/GuiTools.h | 2 +- src/gui/entry/EditEntryWidget.cpp | 16 +- tests/gui/TestGuiFdoSecrets.cpp | 474 ++++++++++++++---- tests/gui/TestGuiFdoSecrets.h | 18 +- 22 files changed, 848 insertions(+), 417 deletions(-) diff --git a/share/translations/keepassxc_en.ts b/share/translations/keepassxc_en.ts index 71529937c..ee955050a 100644 --- a/share/translations/keepassxc_en.ts +++ b/share/translations/keepassxc_en.ts @@ -7880,11 +7880,27 @@ Please consider generating a new key file. - <html><head/><body><p><span style=" font-family:'-apple-system','BlinkMacSystemFont','Segoe UI','Helvetica','Arial','sans-serif','Apple Color Emoji','Segoe UI Emoji'; font-size:14px; color:#24292e; background-color:#ffffff;">This setting does not override disabling recycle bin prompts</span></p></body></html> + Confirm when clients request entry deletion - Confirm when clients request entry deletion + <html><head/><body><p><span style=" + font-family:'-apple-system','BlinkMacSystemFont','Segoe UI','Helvetica','Arial','sans-serif','Apple Color + Emoji','Segoe UI Emoji'; font-size:14px; color:#24292e; background-color:#ffffff;">This setting does + not override disabling recycle bin prompts</span></p></body></html> + + + + + <html><head/><body><p>This improves compatibility with certain applications + which search for password without unlocking the database first.</p><p>But enabling this may also + crash the client if the database can not be unlocked within a certain timeout. (Usually 25s, but may be a + different value set in applications.)</p></body></html> + + + + + Prompt to unlock database before searching diff --git a/src/core/Config.cpp b/src/core/Config.cpp index b586cee05..54fc59c81 100644 --- a/src/core/Config.cpp +++ b/src/core/Config.cpp @@ -178,6 +178,7 @@ static const QHash configStrings = { {Config::FdoSecrets_ShowNotification, {QS("FdoSecrets/ShowNotification"), Roaming, true}}, {Config::FdoSecrets_ConfirmDeleteItem, {QS("FdoSecrets/ConfirmDeleteItem"), Roaming, true}}, {Config::FdoSecrets_ConfirmAccessItem, {QS("FdoSecrets/ConfirmAccessItem"), Roaming, true}}, + {Config::FdoSecrets_UnlockBeforeSearch, {QS("FdoSecrets/UnlockBeforeSearch"), Roaming, true}}, // KeeShare {Config::KeeShare_QuietSuccess, {QS("KeeShare/QuietSuccess"), Roaming, false}}, diff --git a/src/core/Config.h b/src/core/Config.h index c0e3d142b..8e5c14e35 100644 --- a/src/core/Config.h +++ b/src/core/Config.h @@ -155,6 +155,7 @@ public: FdoSecrets_ShowNotification, FdoSecrets_ConfirmDeleteItem, FdoSecrets_ConfirmAccessItem, + FdoSecrets_UnlockBeforeSearch, KeeShare_QuietSuccess, KeeShare_Own, diff --git a/src/fdosecrets/FdoSecretsSettings.cpp b/src/fdosecrets/FdoSecretsSettings.cpp index 58126733a..2f1e95182 100644 --- a/src/fdosecrets/FdoSecretsSettings.cpp +++ b/src/fdosecrets/FdoSecretsSettings.cpp @@ -83,6 +83,16 @@ namespace FdoSecrets config()->set(Config::FdoSecrets_ConfirmAccessItem, confirmAccessItem); } + bool FdoSecretsSettings::unlockBeforeSearch() const + { + return config()->get(Config::FdoSecrets_UnlockBeforeSearch).toBool(); + } + + void FdoSecretsSettings::setUnlockBeforeSearch(bool unlockBeforeSearch) + { + config()->set(Config::FdoSecrets_UnlockBeforeSearch, unlockBeforeSearch); + } + QUuid FdoSecretsSettings::exposedGroup(const QSharedPointer& db) const { return exposedGroup(db.data()); diff --git a/src/fdosecrets/FdoSecretsSettings.h b/src/fdosecrets/FdoSecretsSettings.h index 24a37a8d2..31ab005f6 100644 --- a/src/fdosecrets/FdoSecretsSettings.h +++ b/src/fdosecrets/FdoSecretsSettings.h @@ -44,6 +44,9 @@ namespace FdoSecrets bool confirmAccessItem() const; void setConfirmAccessItem(bool confirmAccessItem); + bool unlockBeforeSearch() const; + void setUnlockBeforeSearch(bool unlockBeforeSearch); + // Per db settings QUuid exposedGroup(const QSharedPointer& db) const; diff --git a/src/fdosecrets/dbus/DBusMgr.cpp b/src/fdosecrets/dbus/DBusMgr.cpp index e60c068aa..1a4b2b73c 100644 --- a/src/fdosecrets/dbus/DBusMgr.cpp +++ b/src/fdosecrets/dbus/DBusMgr.cpp @@ -177,14 +177,12 @@ namespace FdoSecrets // It's still weak and if the application does a prctl(PR_SET_DUMPABLE, 0) this link cannot be accessed. QFileInfo exe(QStringLiteral("/proc/%1/exe").arg(pid)); info.exePath = exe.canonicalFilePath(); - qDebug() << "process" << pid << info.exePath; // /proc/pid/cmdline gives full command line QFile cmdline(QStringLiteral("/proc/%1/cmdline").arg(pid)); if (cmdline.open(QFile::ReadOnly)) { info.command = QString::fromLocal8Bit(cmdline.readAll().replace('\0', ' ')).trimmed(); } - qDebug() << "process" << pid << info.command; // /proc/pid/stat gives ppid, name QFile stat(QStringLiteral("/proc/%1/stat").arg(pid)); diff --git a/src/fdosecrets/dbus/DBusObject.h b/src/fdosecrets/dbus/DBusObject.h index d2314438d..8984cf17e 100644 --- a/src/fdosecrets/dbus/DBusObject.h +++ b/src/fdosecrets/dbus/DBusObject.h @@ -85,7 +85,7 @@ namespace FdoSecrets // Implicitly convert from QDBusError DBusResult(QDBusError::ErrorType error) // NOLINT(google-explicit-constructor) - : QString(QDBusError::errorString(error)) + : QString(error == QDBusError::ErrorType::NoError ? "" : QDBusError::errorString(error)) { } diff --git a/src/fdosecrets/objects/Collection.cpp b/src/fdosecrets/objects/Collection.cpp index c4e24839b..610a4b99e 100644 --- a/src/fdosecrets/objects/Collection.cpp +++ b/src/fdosecrets/objects/Collection.cpp @@ -24,7 +24,9 @@ #include "core/Tools.h" #include "gui/DatabaseWidget.h" +#include "gui/GuiTools.h" +#include #include namespace FdoSecrets @@ -62,9 +64,9 @@ namespace FdoSecrets // delete all items // this has to be done because the backend is actually still there, just we don't expose them - // NOTE: Do NOT use a for loop, because Item::doDelete will remove itself from m_items. + // NOTE: Do NOT use a for loop, because Item::removeFromDBus will remove itself from m_items. while (!m_items.isEmpty()) { - m_items.first()->doDelete(); + m_items.first()->removeFromDBus(); } cleanupConnections(); dbus()->unregisterObject(this); @@ -91,7 +93,7 @@ namespace FdoSecrets void Collection::reloadBackendOrDelete() { if (!reloadBackend()) { - doDelete(); + removeFromDBus(); } } @@ -194,31 +196,22 @@ namespace FdoSecrets return {}; } - DBusResult Collection::remove(const DBusClientPtr& client, PromptBase*& prompt) + DBusResult Collection::remove(PromptBase*& prompt) { auto ret = ensureBackend(); if (ret.err()) { return ret; } - // Delete means close database prompt = PromptBase::Create(service(), this); if (!prompt) { return QDBusError::InternalError; } - if (backendLocked()) { - // this won't raise a dialog, immediate execute - ret = prompt->prompt(client, {}); - if (ret.err()) { - return ret; - } - prompt = nullptr; - } - // defer the close to the prompt return {}; } - DBusResult Collection::searchItems(const StringStringMap& attributes, QList& items) + DBusResult + Collection::searchItems(const DBusClientPtr& client, const StringStringMap& attributes, QList& items) { items.clear(); @@ -226,8 +219,26 @@ namespace FdoSecrets if (ret.err()) { return ret; } - ret = ensureUnlocked(); - if (ret.err()) { + + 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, // because there's no database at all when locked. @@ -258,7 +269,11 @@ namespace FdoSecrets terms << attributeToTerm(it.key(), it.value()); } - const auto foundEntries = EntrySearcher(false, true).search(terms, m_exposedGroup); + constexpr auto caseSensitive = false; + constexpr auto skipProtected = true; + constexpr auto forceSearch = true; + const auto foundEntries = + EntrySearcher(caseSensitive, skipProtected).search(terms, m_exposedGroup, forceSearch); items.reserve(foundEntries.size()); for (const auto& entry : foundEntries) { items << m_entryToItem.value(entry); @@ -298,36 +313,10 @@ namespace FdoSecrets if (ret.err()) { return ret; } - ret = ensureUnlocked(); - if (ret.err()) { - return ret; - } item = nullptr; - QString itemPath; - auto iterAttr = properties.find(DBUS_INTERFACE_SECRET_ITEM + ".Attributes"); - if (iterAttr != properties.end()) { - // the actual value in iterAttr.value() is QDBusArgument, which represents a structure - // and qt has no idea what this corresponds to. - // we thus force a conversion to StringStringMap here. The conversion is registered in - // DBusTypes.cpp - auto attributes = iterAttr.value().value(); - - itemPath = attributes.value(ItemAttributes::PathKey); - - // check existing item using attributes - QList existing; - ret = searchItems(attributes, existing); - if (ret.err()) { - return ret; - } - if (!existing.isEmpty() && replace) { - item = existing.front(); - } - } - - prompt = PromptBase::Create(service(), this, properties, secret, itemPath, item); + prompt = PromptBase::Create(service(), this, properties, secret, replace); if (!prompt) { return QDBusError::InternalError; } @@ -418,7 +407,7 @@ namespace FdoSecrets void Collection::onDatabaseLockChanged() { if (!reloadBackend()) { - doDelete(); + removeFromDBus(); return; } emit collectionLockChanged(backendLocked()); @@ -436,7 +425,7 @@ namespace FdoSecrets auto newGroup = m_backend->database()->rootGroup()->findGroupByUuid(newUuid); if (!newGroup || inRecycleBin(newGroup)) { // no exposed group, delete self - doDelete(); + removeFromDBus(); return; } @@ -505,7 +494,7 @@ namespace FdoSecrets // this has to be done because the backend is actually still there // just we don't expose them for (const auto& item : asConst(m_items)) { - item->doDelete(); + item->removeFromDBus(); } // repopulate @@ -527,7 +516,7 @@ namespace FdoSecrets // forward delete signals connect(entry->group(), &Group::entryAboutToRemove, item, [item](Entry* toBeRemoved) { if (item->backend() == toBeRemoved) { - item->doDelete(); + item->removeFromDBus(); } }); @@ -568,17 +557,26 @@ namespace FdoSecrets { Q_ASSERT(m_backend); - return m_backend->lock(); + // do not call m_backend->lock() directly + // let the service handle locking which handles concurrent calls + return service()->doLockDatabase(m_backend); } void Collection::doUnlock() { Q_ASSERT(m_backend); - service()->doUnlockDatabaseInDialog(m_backend); + return service()->doUnlockDatabaseInDialog(m_backend); } - void Collection::doDelete() + bool Collection::doDelete() + { + Q_ASSERT(m_backend); + + return service()->doCloseDatabase(m_backend); + } + + void Collection::removeFromDBus() { if (!m_backend) { // I'm already deleted @@ -637,9 +635,18 @@ namespace FdoSecrets return !m_backend || !m_backend->database()->isInitialized() || m_backend->isLocked(); } - void Collection::doDeleteEntries(QList entries) + bool Collection::doDeleteEntry(Entry* entry) { - m_backend->deleteEntries(std::move(entries), FdoSecrets::settings()->confirmDeleteItem()); + // Confirm entry removal before moving forward + bool permanent = inRecycleBin(entry) || !m_backend->database()->metadata()->recycleBinEnabled(); + if (FdoSecrets::settings()->confirmDeleteItem() + && !GuiTools::confirmDeleteEntries(m_backend, {entry}, permanent)) { + return false; + } + + auto num = GuiTools::deleteEntriesResolveReferences(m_backend, {entry}, permanent); + + return num != 0; } Group* Collection::findCreateGroupByPath(const QString& groupPath) diff --git a/src/fdosecrets/objects/Collection.h b/src/fdosecrets/objects/Collection.h index 690eb0d99..4bd83d152 100644 --- a/src/fdosecrets/objects/Collection.h +++ b/src/fdosecrets/objects/Collection.h @@ -63,8 +63,10 @@ namespace FdoSecrets Q_INVOKABLE DBUS_PROPERTY DBusResult modified(qulonglong& modified) const; - Q_INVOKABLE DBusResult remove(const DBusClientPtr& client, PromptBase*& prompt); - Q_INVOKABLE DBusResult searchItems(const StringStringMap& attributes, QList& items); + Q_INVOKABLE DBusResult remove(PromptBase*& prompt); + Q_INVOKABLE DBusResult searchItems(const DBusClientPtr& client, + const StringStringMap& attributes, + QList& items); Q_INVOKABLE DBusResult createItem(const QVariantMap& properties, const Secret& secret, bool replace, Item*& item, PromptBase*& prompt); @@ -112,14 +114,18 @@ namespace FdoSecrets public slots: // expose some methods for Prompt to use - bool doLock(); - void doUnlock(); - Item* doNewItem(const DBusClientPtr& client, QString itemPath); - // will remove self - void doDelete(); - // delete the Entry in backend from this collection - void doDeleteEntries(QList entries); + bool doLock(); + // actually only closes database tab in KPXC + bool doDelete(); + // Async, finish signal doneUnlockCollection + void doUnlock(); + + bool doDeleteEntry(Entry* entry); + Item* doNewItem(const DBusClientPtr& client, QString itemPath); + + // Only delete from dbus, will remove self. Do not affect database in KPXC + void removeFromDBus(); // force reload info from backend, potentially delete self bool reloadBackend(); diff --git a/src/fdosecrets/objects/Item.cpp b/src/fdosecrets/objects/Item.cpp index e6c2831b4..9a7d94739 100644 --- a/src/fdosecrets/objects/Item.cpp +++ b/src/fdosecrets/objects/Item.cpp @@ -335,7 +335,14 @@ namespace FdoSecrets return m_backend; } - void Item::doDelete() + bool Item::doDelete() + { + Q_ASSERT(m_backend); + + return collection()->doDeleteEntry(m_backend); + } + + void Item::removeFromDBus() { emit itemAboutToDelete(); diff --git a/src/fdosecrets/objects/Item.h b/src/fdosecrets/objects/Item.h index e11cdb88c..8be4e7d27 100644 --- a/src/fdosecrets/objects/Item.h +++ b/src/fdosecrets/objects/Item.h @@ -91,8 +91,13 @@ namespace FdoSecrets QString path() const; public slots: - void doDelete(); + // will actually delete the entry in KPXC + bool doDelete(); + // Only delete from dbus, will remove self. Do not affect database in KPXC + void removeFromDBus(); + + private slots: /** * Check if the backend is a valid object, send error reply if not. * @return No error if the backend is valid. diff --git a/src/fdosecrets/objects/Prompt.cpp b/src/fdosecrets/objects/Prompt.cpp index 9fa06c25e..58fdfa4e1 100644 --- a/src/fdosecrets/objects/Prompt.cpp +++ b/src/fdosecrets/objects/Prompt.cpp @@ -32,6 +32,7 @@ namespace FdoSecrets { + const PromptResult PromptResult::Pending{PromptResult::AsyncPending}; PromptBase::PromptBase(Service* parent) : DBusObject(parent) @@ -60,19 +61,7 @@ namespace FdoSecrets return qobject_cast(parent()); } - DBusResult PromptBase::dismiss() - { - emit completed(true, ""); - return {}; - } - - DeleteCollectionPrompt::DeleteCollectionPrompt(Service* parent, Collection* coll) - : PromptBase(parent) - , m_collection(coll) - { - } - - DBusResult DeleteCollectionPrompt::prompt(const DBusClientPtr&, const QString& windowId) + DBusResult PromptBase::prompt(const DBusClientPtr& client, const QString& windowId) { if (thread() != QThread::currentThread()) { DBusResult ret; @@ -81,19 +70,62 @@ namespace FdoSecrets return ret; } - if (!m_collection) { - return DBusResult(DBUS_ERROR_SECRET_NO_SUCH_OBJECT); - } - - MessageBox::OverrideParent override(findWindow(windowId)); - // only need to delete in backend, collection will react itself. - auto accepted = service()->doCloseDatabase(m_collection->backend()); - - emit completed(!accepted, ""); - + QWeakPointer weak = client; + // execute the actual prompt method in event loop to avoid block this method + QTimer::singleShot(0, this, [this, weak, windowId]() { + auto c = weak.lock(); + if (!c) { + return; + } + if (m_signalSent) { + return; + } + auto res = promptSync(c, windowId); + if (!res.isPending()) { + finishPrompt(res.isDismiss()); + } + }); return {}; } + DBusResult PromptBase::dismiss() + { + finishPrompt(true); + return {}; + } + + QVariant PromptBase::currentResult() const + { + return ""; + } + + void PromptBase::finishPrompt(bool dismissed) + { + if (m_signalSent) { + return; + } + m_signalSent = true; + emit completed(dismissed, currentResult()); + } + + DeleteCollectionPrompt::DeleteCollectionPrompt(Service* parent, Collection* coll) + : PromptBase(parent) + , m_collection(coll) + { + } + + PromptResult DeleteCollectionPrompt::promptSync(const DBusClientPtr&, const QString& windowId) + { + MessageBox::OverrideParent override(findWindow(windowId)); + + // if m_collection is already gone then treat as deletion accepted + auto accepted = true; + if (m_collection) { + accepted = m_collection->doDelete(); + } + return PromptResult::accepted(accepted); + } + CreateCollectionPrompt::CreateCollectionPrompt(Service* parent, QVariantMap properties, QString alias) : PromptBase(parent) , m_properties(std::move(properties)) @@ -101,43 +133,45 @@ namespace FdoSecrets { } - DBusResult CreateCollectionPrompt::prompt(const DBusClientPtr&, const QString& windowId) + QVariant CreateCollectionPrompt::currentResult() const { - if (thread() != QThread::currentThread()) { - DBusResult ret; - QMetaObject::invokeMethod( - this, "prompt", Qt::BlockingQueuedConnection, Q_ARG(QString, windowId), Q_RETURN_ARG(DBusResult, ret)); - return ret; - } + return QVariant::fromValue(DBusMgr::objectPathSafe(m_coll)); + } + PromptResult CreateCollectionPrompt::promptSync(const DBusClientPtr&, const QString& windowId) + { MessageBox::OverrideParent override(findWindow(windowId)); - auto coll = service()->doNewDatabase(); - if (!coll) { - return dismiss(); - } - - auto ret = coll->setProperties(m_properties); + bool created = false; + // collection with the alias may be created since the prompt was created + auto ret = service()->readAlias(m_alias, m_coll); if (ret.err()) { - coll->doDelete(); - return dismiss(); + return ret; + } + if (!m_coll) { + created = true; + m_coll = service()->doNewDatabase(); + if (!m_coll) { + return PromptResult::accepted(false); + } + } + ret = m_coll->setProperties(m_properties); + if (ret.err()) { + if (created) { + m_coll->removeFromDBus(); + } + return ret; } if (!m_alias.isEmpty()) { - ret = coll->addAlias(m_alias); + ret = m_coll->addAlias(m_alias); if (ret.err()) { - coll->doDelete(); - return dismiss(); + if (created) { + m_coll->removeFromDBus(); + } + return ret; } } - emit completed(false, QVariant::fromValue(coll->objectPath())); - - return {}; - } - - DBusResult CreateCollectionPrompt::dismiss() - { - emit completed(true, QVariant::fromValue(DBusMgr::objectPathSafe(nullptr))); return {}; } @@ -150,35 +184,24 @@ namespace FdoSecrets } } - DBusResult LockCollectionsPrompt::prompt(const DBusClientPtr&, const QString& windowId) + QVariant LockCollectionsPrompt::currentResult() const { - if (thread() != QThread::currentThread()) { - DBusResult ret; - QMetaObject::invokeMethod( - this, "prompt", Qt::BlockingQueuedConnection, Q_ARG(QString, windowId), Q_RETURN_ARG(DBusResult, ret)); - return ret; - } + return QVariant::fromValue(m_locked); + } + PromptResult LockCollectionsPrompt::promptSync(const DBusClientPtr&, const QString& windowId) + { MessageBox::OverrideParent override(findWindow(windowId)); for (const auto& c : asConst(m_collections)) { if (c) { - if (!c->doLock()) { - return dismiss(); + auto accepted = c->doLock(); + if (accepted) { + m_locked << c->objectPath(); } - m_locked << c->objectPath(); } } - - emit completed(false, QVariant::fromValue(m_locked)); - - return {}; - } - - DBusResult LockCollectionsPrompt::dismiss() - { - emit completed(true, QVariant::fromValue(m_locked)); - return {}; + return PromptResult::accepted(m_locked.size() == m_collections.size()); } UnlockPrompt::UnlockPrompt(Service* parent, const QSet& colls, const QSet& items) @@ -194,15 +217,13 @@ namespace FdoSecrets } } - DBusResult UnlockPrompt::prompt(const DBusClientPtr& client, const QString& windowId) + QVariant UnlockPrompt::currentResult() const { - if (thread() != QThread::currentThread()) { - DBusResult ret; - QMetaObject::invokeMethod( - this, "prompt", Qt::BlockingQueuedConnection, Q_ARG(QString, windowId), Q_RETURN_ARG(DBusResult, ret)); - return ret; - } + return QVariant::fromValue(m_unlocked); + } + PromptResult UnlockPrompt::promptSync(const DBusClientPtr& client, const QString& windowId) + { MessageBox::OverrideParent override(findWindow(windowId)); // for use in unlockItems @@ -214,6 +235,7 @@ namespace FdoSecrets for (const auto& c : asConst(m_collections)) { if (c) { // doUnlock is nonblocking, execution will continue in collectionUnlockFinished + // it is ok to call doUnlock multiple times before it's actually unlocked by the user c->doUnlock(); waitingForCollections = true; } @@ -222,11 +244,10 @@ namespace FdoSecrets // unlock items directly if no collection unlocking pending // o.w. do it in collectionUnlockFinished if (!waitingForCollections) { - // do not block the current method - QTimer::singleShot(0, this, &UnlockPrompt::unlockItems); + unlockItems(); } - return {}; + return PromptResult::Pending; } void UnlockPrompt::collectionUnlockFinished(bool accepted) @@ -248,8 +269,7 @@ namespace FdoSecrets m_unlocked << coll->objectPath(); } else { m_numRejected += 1; - // no longer need to unlock the item if its containing collection - // didn't unlock. + // no longer need to unlock the item if its containing collection didn't unlock. m_items.remove(coll); } @@ -270,7 +290,7 @@ namespace FdoSecrets // flatten to list of entries QList entries; - for (const auto& itemsPerColl : m_items.values()) { + for (const auto& itemsPerColl : asConst(m_items)) { for (const auto& item : itemsPerColl) { if (!item) { m_numRejected += 1; @@ -306,6 +326,7 @@ namespace FdoSecrets auto client = m_client.lock(); if (!client) { // client already gone + qDebug() << "DBus client gone before item unlocking finish"; return; } for (auto it = decisions.constBegin(); it != decisions.constEnd(); ++it) { @@ -326,13 +347,7 @@ namespace FdoSecrets } // if anything is not unlocked, treat the whole prompt as dismissed // so the client has a chance to handle the error - emit completed(m_numRejected > 0, QVariant::fromValue(m_unlocked)); - } - - DBusResult UnlockPrompt::dismiss() - { - emit completed(true, QVariant::fromValue(m_unlocked)); - return {}; + finishPrompt(m_numRejected > 0); } DeleteItemPrompt::DeleteItemPrompt(Service* parent, Item* item) @@ -341,117 +356,114 @@ namespace FdoSecrets { } - DBusResult DeleteItemPrompt::prompt(const DBusClientPtr&, const QString& windowId) + PromptResult DeleteItemPrompt::promptSync(const DBusClientPtr&, const QString& windowId) { - if (thread() != QThread::currentThread()) { - DBusResult ret; - QMetaObject::invokeMethod( - this, "prompt", Qt::BlockingQueuedConnection, Q_ARG(QString, windowId), Q_RETURN_ARG(DBusResult, ret)); - return ret; - } - MessageBox::OverrideParent override(findWindow(windowId)); - // delete item's backend. Item will be notified after the backend is deleted. + // if m_item is gone, assume it's already deleted + bool deleted = true; if (m_item) { - m_item->collection()->doDeleteEntries({m_item->backend()}); + deleted = m_item->doDelete(); } - - emit completed(false, ""); - - return {}; + return PromptResult::accepted(deleted); } CreateItemPrompt::CreateItemPrompt(Service* parent, Collection* coll, QVariantMap properties, Secret secret, - QString itemPath, - Item* existing) + bool replace) : PromptBase(parent) , m_coll(coll) , m_properties(std::move(properties)) , m_secret(std::move(secret)) - , m_itemPath(std::move(itemPath)) - , m_item(existing) + , m_replace(replace) + , m_item(nullptr) // session aliveness also need to be tracked, for potential use later in updateItem , m_sess(m_secret.session) { } - DBusResult CreateItemPrompt::prompt(const DBusClientPtr& client, const QString& windowId) + QVariant CreateItemPrompt::currentResult() const { - if (thread() != QThread::currentThread()) { - DBusResult ret; - QMetaObject::invokeMethod( - this, "prompt", Qt::BlockingQueuedConnection, Q_ARG(QString, windowId), Q_RETURN_ARG(DBusResult, ret)); - return ret; + return QVariant::fromValue(DBusMgr::objectPathSafe(m_item)); + } + + PromptResult CreateItemPrompt::promptSync(const DBusClientPtr& client, const QString& windowId) + { + if (!m_coll) { + return PromptResult::accepted(false); } - MessageBox::OverrideParent override(findWindow(windowId)); - - if (!m_coll) { - return dismiss(); + bool locked = true; + auto ret = m_coll->locked(locked); + if (locked) { + // collection was locked + return DBusResult{DBUS_ERROR_SECRET_IS_LOCKED}; } // save a weak reference to the client which may be used asynchronously later m_client = client; - // the item doesn't exists yet, create it + // get itemPath to create item and + // try finding an existing item using attributes + QString itemPath{}; + auto iterAttr = m_properties.find(DBUS_INTERFACE_SECRET_ITEM + ".Attributes"); + if (iterAttr != m_properties.end()) { + // the actual value in iterAttr.value() is QDBusArgument, which represents a structure + // and qt has no idea what this corresponds to. + // we thus force a conversion to StringStringMap here. The conversion is registered in + // DBusTypes.cpp + auto attributes = iterAttr.value().value(); + + itemPath = attributes.value(ItemAttributes::PathKey); + + // check existing item using attributes + QList existing; + ret = m_coll->searchItems(client, attributes, existing); + if (ret.err()) { + return ret; + } + if (!existing.isEmpty() && m_replace) { + m_item = existing.front(); + } + } + if (!m_item) { - m_item = m_coll->doNewItem(client, m_itemPath); + // the item doesn't exist yet, create it + m_item = m_coll->doNewItem(client, itemPath); if (!m_item) { // may happen if entry somehow ends up in recycle bin - return DBusResult(DBUS_ERROR_SECRET_NO_SUCH_OBJECT); + return DBusResult{DBUS_ERROR_SECRET_NO_SUCH_OBJECT}; } + } - auto ret = updateItem(); - if (ret.err()) { - m_item->doDelete(); - return ret; + // the item may be locked due to authorization + ret = m_item->locked(client, locked); + if (ret.err()) { + return ret; + } + if (locked) { + // give the user a chance to unlock the item + auto prompt = PromptBase::Create(service(), QSet{}, QSet{m_item}); + if (!prompt) { + return DBusResult{QDBusError::InternalError}; } - emit completed(false, QVariant::fromValue(m_item->objectPath())); - } else { - bool locked = false; - auto ret = m_item->locked(client, locked); + // postpone anything after the confirmation + connect(prompt, &PromptBase::completed, this, [this]() { + auto res = updateItem(); + finishPrompt(res.err()); + }); + + ret = prompt->prompt(client, windowId); if (ret.err()) { return ret; } - if (locked) { - // give the user a chance to unlock the item - auto prompt = PromptBase::Create(service(), QSet{}, QSet{m_item}); - if (!prompt) { - return QDBusError::InternalError; - } - // postpone anything after the confirmation - connect(prompt, &PromptBase::completed, this, &CreateItemPrompt::itemUnlocked); - return prompt->prompt(client, windowId); - } else { - ret = updateItem(); - if (ret.err()) { - return ret; - } - emit completed(false, QVariant::fromValue(m_item->objectPath())); - } + return PromptResult::Pending; } - return {}; - } - DBusResult CreateItemPrompt::dismiss() - { - emit completed(true, QVariant::fromValue(DBusMgr::objectPathSafe(nullptr))); - return {}; - } - - void CreateItemPrompt::itemUnlocked(bool dismissed, const QVariant& result) - { - auto unlocked = result.value>(); - if (!unlocked.isEmpty()) { - // in theory we should check if the object path matches m_item, but a mismatch should not happen, - // because we control the unlock prompt ourselves - updateItem(); - } - emit completed(dismissed, QVariant::fromValue(DBusMgr::objectPathSafe(m_item))); + // the item can be updated directly + return updateItem(); } DBusResult CreateItemPrompt::updateItem() diff --git a/src/fdosecrets/objects/Prompt.h b/src/fdosecrets/objects/Prompt.h index e3de1916a..2e9ffed9c 100644 --- a/src/fdosecrets/objects/Prompt.h +++ b/src/fdosecrets/objects/Prompt.h @@ -32,14 +32,60 @@ namespace FdoSecrets class Service; + // a simple helper class to auto convert + // true/false, DBusResult and Pending values + class PromptResult + { + enum Value + { + Accepted, + Dismissed, + AsyncPending, + }; + const Value value; + + explicit PromptResult(Value v) noexcept + : value(v) + { + } + explicit PromptResult(bool accepted) + : value(accepted ? Accepted : Dismissed) + { + } + + public: + PromptResult() + : PromptResult(true) + { + } + PromptResult(const DBusResult& res) // NOLINT(google-explicit-constructor) + : PromptResult(res.ok()) + { + } + + static const PromptResult Pending; + static PromptResult accepted(bool accepted) + { + return PromptResult{accepted}; + } + + bool isDismiss() const + { + return value == Dismissed; + } + bool isPending() const + { + return value == AsyncPending; + } + }; + class PromptBase : public DBusObject { Q_OBJECT Q_CLASSINFO("D-Bus Interface", DBUS_INTERFACE_SECRET_PROMPT_LITERAL) public: - Q_INVOKABLE virtual DBusResult prompt(const DBusClientPtr& client, const QString& windowId) = 0; - - Q_INVOKABLE virtual DBusResult dismiss(); + Q_INVOKABLE DBusResult prompt(const DBusClientPtr& client, const QString& windowId); + Q_INVOKABLE DBusResult dismiss(); template static PromptBase* Create(Service* parent, ARGS&&... args) { @@ -57,8 +103,15 @@ namespace FdoSecrets protected: explicit PromptBase(Service* parent); + virtual PromptResult promptSync(const DBusClientPtr& client, const QString& windowId) = 0; + virtual QVariant currentResult() const; + QWindow* findWindow(const QString& windowId); Service* service() const; + void finishPrompt(bool dismissed); + + private: + bool m_signalSent = false; }; class Collection; @@ -66,14 +119,11 @@ namespace FdoSecrets class DeleteCollectionPrompt : public PromptBase { Q_OBJECT + friend class PromptBase; explicit DeleteCollectionPrompt(Service* parent, Collection* coll); - public: - DBusResult prompt(const DBusClientPtr& client, const QString& windowId) override; - - private: - friend class PromptBase; + PromptResult promptSync(const DBusClientPtr& client, const QString& windowId) override; QPointer m_collection; }; @@ -81,32 +131,27 @@ namespace FdoSecrets class CreateCollectionPrompt : public PromptBase { Q_OBJECT + friend class PromptBase; explicit CreateCollectionPrompt(Service* parent, QVariantMap properties, QString alias); - public: - DBusResult prompt(const DBusClientPtr& client, const QString& windowId) override; - DBusResult dismiss() override; - - private: - friend class PromptBase; + PromptResult promptSync(const DBusClientPtr& client, const QString& windowId) override; + QVariant currentResult() const override; QVariantMap m_properties; QString m_alias; + Collection* m_coll{}; }; class LockCollectionsPrompt : public PromptBase { Q_OBJECT + friend class PromptBase; explicit LockCollectionsPrompt(Service* parent, const QList& colls); - public: - DBusResult prompt(const DBusClientPtr& client, const QString& windowId) override; - DBusResult dismiss() override; - - private: - friend class PromptBase; + PromptResult promptSync(const DBusClientPtr& client, const QString& windowId) override; + QVariant currentResult() const override; QList> m_collections; QList m_locked; @@ -116,22 +161,17 @@ namespace FdoSecrets class UnlockPrompt : public PromptBase { Q_OBJECT + friend class PromptBase; explicit UnlockPrompt(Service* parent, const QSet& colls, const QSet& items); - public: - DBusResult prompt(const DBusClientPtr& client, const QString& windowId) override; - DBusResult dismiss() override; + PromptResult promptSync(const DBusClientPtr& client, const QString& windowId) override; + QVariant currentResult() const override; - private slots: void collectionUnlockFinished(bool accepted); void itemUnlockFinished(const QHash& results); - - private: void unlockItems(); - friend class PromptBase; - static constexpr auto FdoSecretsBackend = "FdoSecretsBackend"; QList> m_collections; @@ -148,14 +188,11 @@ namespace FdoSecrets class DeleteItemPrompt : public PromptBase { Q_OBJECT + friend class PromptBase; explicit DeleteItemPrompt(Service* parent, Item* item); - public: - DBusResult prompt(const DBusClientPtr& client, const QString& windowId) override; - - private: - friend class PromptBase; + PromptResult promptSync(const DBusClientPtr& client, const QString& windowId) override; QPointer m_item; }; @@ -163,29 +200,24 @@ namespace FdoSecrets class CreateItemPrompt : public PromptBase { Q_OBJECT + friend class PromptBase; explicit CreateItemPrompt(Service* parent, Collection* coll, QVariantMap properties, Secret secret, - QString itemPath, - Item* existing); + bool replace); - public: - DBusResult prompt(const DBusClientPtr& client, const QString& windowId) override; - DBusResult dismiss() override; - private slots: - void itemUnlocked(bool dismissed, const QVariant& result); + PromptResult promptSync(const DBusClientPtr& client, const QString& windowId) override; + QVariant currentResult() const override; - private: DBusResult updateItem(); - friend class PromptBase; - QPointer m_coll; QVariantMap m_properties; Secret m_secret; - QString m_itemPath; + bool m_replace; + QPointer m_item; QPointer m_sess; diff --git a/src/fdosecrets/objects/Service.cpp b/src/fdosecrets/objects/Service.cpp index dc42b632e..60e057e7d 100644 --- a/src/fdosecrets/objects/Service.cpp +++ b/src/fdosecrets/objects/Service.cpp @@ -52,8 +52,10 @@ namespace FdoSecrets , m_databases(std::move(dbTabs)) , m_insideEnsureDefaultAlias(false) { - connect( - m_databases, &DatabaseTabWidget::databaseUnlockDialogFinished, this, &Service::doneUnlockDatabaseInDialog); + connect(m_databases, + &DatabaseTabWidget::databaseUnlockDialogFinished, + this, + &Service::onDatabaseUnlockDialogFinished); } Service::~Service() = default; @@ -86,7 +88,7 @@ namespace FdoSecrets { // The Collection will monitor the database's exposed group. // When the Collection finds that no exposed group, it will delete itself. - // Thus the service also needs to monitor it and recreate the collection if the user changes + // Thus, the service also needs to monitor it and recreate the collection if the user changes // from no exposed to exposed something. connect(dbWidget, &DatabaseWidget::databaseReplaced, this, [this, dbWidget]() { monitorDatabaseExposedGroup(dbWidget); @@ -119,16 +121,16 @@ namespace FdoSecrets // m_backend may already be reset to nullptr // We want to remove the collection object from dbus as early as possible, to avoid // race conditions when deleteLater was called on the m_backend, but not delivered yet, - // and new method calls from dbus occurred. Therefore we can't rely on the destroyed + // and new method calls from dbus occurred. Therefore, we can't rely on the destroyed // signal on m_backend. // bind to coll lifespan connect(m_databases.data(), &DatabaseTabWidget::databaseClosed, coll, [coll](const QString& filePath) { if (filePath == coll->backendFilePath()) { - coll->doDelete(); + coll->removeFromDBus(); } }); - // actual load, must after updates to m_collections, because the reload may trigger + // actual load, must after updates to m_collections, because reloading may trigger // another onDatabaseTabOpen, and m_collections will be used to prevent recursion. if (!coll->reloadBackend()) { // error in dbus @@ -250,7 +252,7 @@ namespace FdoSecrets for (const auto& coll : asConst(colls)) { QList items; - ret = coll->searchItems(attributes, items); + ret = coll->searchItems(client, attributes, items); if (ret.err()) { return ret; } @@ -282,7 +284,7 @@ namespace FdoSecrets itemsToUnlock.reserve(objects.size()); for (const auto& obj : asConst(objects)) { - // the object is either an item or an collection + // the object is either an item or a collection auto item = qobject_cast(obj); auto coll = item ? item->collection() : qobject_cast(obj); // either way there should be a collection @@ -317,6 +319,7 @@ namespace FdoSecrets } } + prompt = nullptr; if (!collectionsToUnlock.isEmpty() || !itemsToUnlock.isEmpty()) { prompt = PromptBase::Create(this, collectionsToUnlock, itemsToUnlock); if (!prompt) { @@ -493,9 +496,44 @@ namespace FdoSecrets m_plugin->emitRequestSwitchToDatabases(); } + bool Service::doLockDatabase(DatabaseWidget* dbWidget) + { + // return immediately if the db is already unlocked + if (dbWidget && dbWidget->isLocked()) { + return true; + } + + // mark the db as being unlocked to prevent multiple dialogs for the same db + if (m_lockingDb.contains(dbWidget)) { + return true; + } + m_lockingDb.insert(dbWidget); + auto ret = dbWidget->lock(); + m_lockingDb.remove(dbWidget); + + return ret; + } + void Service::doUnlockDatabaseInDialog(DatabaseWidget* dbWidget) { + // return immediately if the db is already unlocked + if (dbWidget && !dbWidget->isLocked()) { + emit doneUnlockDatabaseInDialog(true, dbWidget); + return; + } + + // mark the db as being unlocked to prevent multiple dialogs for the same db + if (m_unlockingDb.contains(dbWidget)) { + return; + } + m_unlockingDb.insert(dbWidget); + m_databases->unlockDatabaseInDialog(dbWidget, DatabaseOpenDialog::Intent::None); } + void Service::onDatabaseUnlockDialogFinished(bool accepted, DatabaseWidget* dbWidget) + { + m_unlockingDb.remove(dbWidget); + emit doneUnlockDatabaseInDialog(accepted, dbWidget); + } } // namespace FdoSecrets diff --git a/src/fdosecrets/objects/Service.h b/src/fdosecrets/objects/Service.h index 84eee230d..422a3ca94 100644 --- a/src/fdosecrets/objects/Service.h +++ b/src/fdosecrets/objects/Service.h @@ -96,8 +96,8 @@ namespace FdoSecrets /** * Finish signal for async action doUnlockDatabaseInDialog - * @param accepted If false, the action is canceled by the user - * @param dbWidget The unlocked the dbWidget if succeed + * @param accepted If false, the action is cancelled by the user + * @param dbWidget The dbWidget the action is on */ void doneUnlockDatabaseInDialog(bool accepted, DatabaseWidget* dbWidget); @@ -114,6 +114,7 @@ namespace FdoSecrets } public slots: + bool doLockDatabase(DatabaseWidget* dbWidget); bool doCloseDatabase(DatabaseWidget* dbWidget); Collection* doNewDatabase(); void doSwitchToDatabaseSettings(DatabaseWidget* dbWidget); @@ -135,6 +136,8 @@ namespace FdoSecrets void onCollectionAliasRemoved(const QString& alias); + void onDatabaseUnlockDialogFinished(bool accepted, DatabaseWidget* dbWidget); + private: bool initialize(); @@ -153,16 +156,18 @@ namespace FdoSecrets Collection* findCollection(const DatabaseWidget* db) const; private: - FdoSecretsPlugin* m_plugin; - QPointer m_databases; + FdoSecretsPlugin* m_plugin{nullptr}; + QPointer m_databases{}; - QHash m_aliases; - QList m_collections; - QHash m_dbToCollection; + QHash m_aliases{}; + QList m_collections{}; + QHash m_dbToCollection{}; - QList m_sessions; + QList m_sessions{}; - bool m_insideEnsureDefaultAlias; + bool m_insideEnsureDefaultAlias{false}; + QSet m_unlockingDb{}; // list of db being unlocking + QSet m_lockingDb{}; // list of db being locking }; } // namespace FdoSecrets diff --git a/src/fdosecrets/widgets/SettingsWidgetFdoSecrets.cpp b/src/fdosecrets/widgets/SettingsWidgetFdoSecrets.cpp index 5ab22749b..842a40b98 100644 --- a/src/fdosecrets/widgets/SettingsWidgetFdoSecrets.cpp +++ b/src/fdosecrets/widgets/SettingsWidgetFdoSecrets.cpp @@ -255,6 +255,7 @@ void SettingsWidgetFdoSecrets::loadSettings() m_ui->showNotification->setChecked(FdoSecrets::settings()->showNotification()); m_ui->confirmDeleteItem->setChecked(FdoSecrets::settings()->confirmDeleteItem()); m_ui->confirmAccessItem->setChecked(FdoSecrets::settings()->confirmAccessItem()); + m_ui->unlockBeforeSearch->setChecked(FdoSecrets::settings()->unlockBeforeSearch()); } void SettingsWidgetFdoSecrets::saveSettings() @@ -263,6 +264,7 @@ void SettingsWidgetFdoSecrets::saveSettings() FdoSecrets::settings()->setShowNotification(m_ui->showNotification->isChecked()); FdoSecrets::settings()->setConfirmDeleteItem(m_ui->confirmDeleteItem->isChecked()); FdoSecrets::settings()->setConfirmAccessItem(m_ui->confirmAccessItem->isChecked()); + FdoSecrets::settings()->setUnlockBeforeSearch(m_ui->unlockBeforeSearch->isChecked()); } void SettingsWidgetFdoSecrets::showEvent(QShowEvent* event) diff --git a/src/fdosecrets/widgets/SettingsWidgetFdoSecrets.ui b/src/fdosecrets/widgets/SettingsWidgetFdoSecrets.ui index 7034d7bd6..bed666676 100644 --- a/src/fdosecrets/widgets/SettingsWidgetFdoSecrets.ui +++ b/src/fdosecrets/widgets/SettingsWidgetFdoSecrets.ui @@ -72,7 +72,11 @@ - <html><head/><body><p><span style=" font-family:'-apple-system','BlinkMacSystemFont','Segoe UI','Helvetica','Arial','sans-serif','Apple Color Emoji','Segoe UI Emoji'; font-size:14px; color:#24292e; background-color:#ffffff;">This setting does not override disabling recycle bin prompts</span></p></body></html> + <html><head/><body><p><span style=" + font-family:'-apple-system','BlinkMacSystemFont','Segoe UI','Helvetica','Arial','sans-serif','Apple Color + Emoji','Segoe UI Emoji'; font-size:14px; color:#24292e; background-color:#ffffff;">This setting does + not override disabling recycle bin prompts</span></p></body></html> + Confirm when clients request entry deletion @@ -82,6 +86,23 @@ + + + + <html><head/><body><p>This improves compatibility with certain applications + which search for password without unlocking the database first.</p><p>But enabling this may also + crash the client if the database can not be unlocked within a certain timeout. (Usually 25s, but may be a + different value set in applications.)</p></body></html> + + + + Prompt to unlock database before searching + + + true + + + diff --git a/src/gui/GuiTools.cpp b/src/gui/GuiTools.cpp index 7aeeeb7b0..30963b374 100644 --- a/src/gui/GuiTools.cpp +++ b/src/gui/GuiTools.cpp @@ -66,10 +66,10 @@ namespace GuiTools } } - void deleteEntriesResolveReferences(QWidget* parent, const QList& entries, bool permanent) + size_t deleteEntriesResolveReferences(QWidget* parent, const QList& entries, bool permanent) { if (!parent || entries.isEmpty()) { - return; + return 0; } QList selectedEntries; @@ -116,5 +116,6 @@ namespace GuiTools entry->database()->recycleEntry(entry); } } + return selectedEntries.size(); } } // namespace GuiTools diff --git a/src/gui/GuiTools.h b/src/gui/GuiTools.h index 14a54ab75..814537382 100644 --- a/src/gui/GuiTools.h +++ b/src/gui/GuiTools.h @@ -26,6 +26,6 @@ class Entry; namespace GuiTools { bool confirmDeleteEntries(QWidget* parent, const QList& entries, bool permanent); - void deleteEntriesResolveReferences(QWidget* parent, const QList& entries, bool permanent); + size_t deleteEntriesResolveReferences(QWidget* parent, const QList& entries, bool permanent); } // namespace GuiTools #endif // KEEPASSXC_GUITOOLS_H diff --git a/src/gui/entry/EditEntryWidget.cpp b/src/gui/entry/EditEntryWidget.cpp index e736efaad..f9527966b 100644 --- a/src/gui/entry/EditEntryWidget.cpp +++ b/src/gui/entry/EditEntryWidget.cpp @@ -69,20 +69,20 @@ EditEntryWidget::EditEntryWidget(QWidget* parent) , m_browserUi(new Ui::EditEntryWidgetBrowser()) , m_attachments(new EntryAttachments()) , m_customData(new CustomData()) - , m_mainWidget(new QScrollArea()) - , m_advancedWidget(new QWidget()) - , m_iconsWidget(new EditWidgetIcons()) - , m_autoTypeWidget(new QWidget()) + , m_mainWidget(new QScrollArea(this)) + , m_advancedWidget(new QWidget(this)) + , m_iconsWidget(new EditWidgetIcons(this)) + , m_autoTypeWidget(new QWidget(this)) #ifdef WITH_XC_SSHAGENT - , m_sshAgentWidget(new QWidget()) + , m_sshAgentWidget(new QWidget(this)) #endif #ifdef WITH_XC_BROWSER , m_browserSettingsChanged(false) - , m_browserWidget(new QWidget()) + , m_browserWidget(new QWidget(this)) , m_additionalURLsDataModel(new EntryURLModel(this)) #endif - , m_editWidgetProperties(new EditWidgetProperties()) - , m_historyWidget(new QWidget()) + , m_editWidgetProperties(new EditWidgetProperties(this)) + , m_historyWidget(new QWidget(this)) , m_entryAttributes(new EntryAttributes(this)) , m_attributesModel(new EntryAttributesModel(m_advancedWidget)) , m_historyModel(new EntryHistoryModel(this)) diff --git a/tests/gui/TestGuiFdoSecrets.cpp b/tests/gui/TestGuiFdoSecrets.cpp index 5e8bd01cd..ffdcd338f 100644 --- a/tests/gui/TestGuiFdoSecrets.cpp +++ b/tests/gui/TestGuiFdoSecrets.cpp @@ -186,7 +186,7 @@ void TestGuiFdoSecrets::init() // make sure window is activated or focus tests may fail m_mainWindow->activateWindow(); - QApplication::processEvents(); + processEvents(); // open and unlock the database m_tabWidget->addDatabaseTab(m_dbFile->fileName(), false, "a"); @@ -202,6 +202,7 @@ void TestGuiFdoSecrets::init() void TestGuiFdoSecrets::cleanup() { // restore to default settings + FdoSecrets::settings()->setUnlockBeforeSearch(false); FdoSecrets::settings()->setShowNotification(false); FdoSecrets::settings()->setConfirmAccessItem(false); FdoSecrets::settings()->setEnabled(false); @@ -213,8 +214,14 @@ void TestGuiFdoSecrets::cleanup() for (int i = 0; i != m_tabWidget->count(); ++i) { m_tabWidget->databaseWidgetFromIndex(i)->database()->markAsClean(); } + + // Close any dialogs + while (auto w = QApplication::activeModalWidget()) { + w->close(); + } + VERIFY(m_tabWidget->closeAllDatabaseTabs()); - QApplication::processEvents(); + processEvents(); if (m_dbFile) { m_dbFile->remove(); @@ -250,7 +257,7 @@ void TestGuiFdoSecrets::testServiceEnable() VERIFY(sigError.isEmpty()); COMPARE(sigStarted.size(), 1); - QApplication::processEvents(); + processEvents(); VERIFY(QDBusConnection::sessionBus().interface()->isServiceRegistered(DBUS_SERVICE_SECRET)); @@ -288,9 +295,11 @@ void TestGuiFdoSecrets::testServiceSearch() auto item = getFirstItem(coll); VERIFY(item); - auto entries = m_db->rootGroup()->entriesRecursive(false); - VERIFY(!entries.isEmpty()); - const auto& entry = entries.first(); + auto itemObj = m_plugin->dbus()->pathToObject(QDBusObjectPath(item->path())); + VERIFY(itemObj); + auto entry = itemObj->backend(); + VERIFY(entry); + entry->attributes()->set("fdosecrets-test", "1"); entry->attributes()->set("fdosecrets-test-protected", "2", true); const QString crazyKey = "_a:bc&-+'-e%12df_d"; @@ -336,6 +345,72 @@ void TestGuiFdoSecrets::testServiceSearch() } } +void TestGuiFdoSecrets::testServiceSearchBlockingUnlock() +{ + auto service = enableService(); + VERIFY(service); + auto coll = getDefaultCollection(service); + VERIFY(coll); + + auto entries = m_db->rootGroup()->entriesRecursive(); + VERIFY(!entries.isEmpty()); + // assumes the db is not empty + auto title = entries.first()->title(); + + // NOTE: entries are no longer valid after locking + lockDatabaseInBackend(); + + // when database is locked, nothing is returned + FdoSecrets::settings()->setUnlockBeforeSearch(false); + { + DBUS_GET2(unlocked, locked, service->SearchItems({{"Title", title}})); + COMPARE(locked, {}); + COMPARE(unlocked, {}); + } + + // when database is locked, nothing is returned + FdoSecrets::settings()->setUnlockBeforeSearch(true); + { + // SearchItems will block because the blocking wait is implemented + // using a local QEventLoop. + // so we do a little trick here to get the return value back + bool unlockDialogWorks = false; + QTimer::singleShot(50, [&]() { unlockDialogWorks = driveUnlockDialog(); }); + + DBUS_GET2(unlocked, locked, service->SearchItems({{"Title", title}})); + VERIFY(unlockDialogWorks); + COMPARE(locked, {}); + COMPARE(unlocked.size(), 1); + auto item = getProxy(unlocked.first()); + DBUS_COMPARE(item->label(), title); + } +} + +void TestGuiFdoSecrets::testServiceSearchForce() +{ + auto service = enableService(); + VERIFY(service); + auto coll = getDefaultCollection(service); + VERIFY(coll); + auto item = getFirstItem(coll); + VERIFY(item); + + auto itemObj = m_plugin->dbus()->pathToObject(QDBusObjectPath(item->path())); + VERIFY(itemObj); + auto entry = itemObj->backend(); + VERIFY(entry); + + // fdosecrets should still find the item even if searching is disabled + entry->group()->setSearchingEnabled(Group::Disable); + + // search by title + { + DBUS_GET2(unlocked, locked, service->SearchItems({{"Title", entry->title()}})); + COMPARE(locked, {}); + COMPARE(unlocked, {QDBusObjectPath(item->path())}); + } +} + void TestGuiFdoSecrets::testServiceUnlock() { lockDatabaseInBackend(); @@ -362,46 +437,83 @@ void TestGuiFdoSecrets::testServiceUnlock() VERIFY(spyPromptCompleted.isValid()); // nothing is unlocked yet - QTRY_COMPARE(spyPromptCompleted.count(), 0); + VERIFY(waitForSignal(spyPromptCompleted, 0)); DBUS_COMPARE(coll->locked(), true); - // drive the prompt + // show the prompt DBUS_VERIFY(prompt->Prompt("")); // still not unlocked before user action - QTRY_COMPARE(spyPromptCompleted.count(), 0); + VERIFY(waitForSignal(spyPromptCompleted, 0)); DBUS_COMPARE(coll->locked(), true); - // interact with the dialog - QApplication::processEvents(); - { - auto dbOpenDlg = m_tabWidget->findChild(); - VERIFY(dbOpenDlg); - auto editPassword = dbOpenDlg->findChild("editPassword"); - VERIFY(editPassword); - editPassword->setFocus(); - QTest::keyClicks(editPassword, "a"); - QTest::keyClick(editPassword, Qt::Key_Enter); - } - QApplication::processEvents(); + VERIFY(driveUnlockDialog()); - // unlocked - DBUS_COMPARE(coll->locked(), false); - - QTRY_COMPARE(spyPromptCompleted.count(), 1); + VERIFY(waitForSignal(spyPromptCompleted, 1)); { auto args = spyPromptCompleted.takeFirst(); COMPARE(args.size(), 2); COMPARE(args.at(0).toBool(), false); COMPARE(getSignalVariantArgument>(args.at(1)), {QDBusObjectPath(coll->path())}); } - QTRY_COMPARE(spyCollectionCreated.count(), 0); + + // check unlocked *AFTER* the prompt signal + DBUS_COMPARE(coll->locked(), false); + + VERIFY(waitForSignal(spyCollectionCreated, 0)); QTRY_VERIFY(!spyCollectionChanged.isEmpty()); for (const auto& args : spyCollectionChanged) { COMPARE(args.size(), 1); COMPARE(args.at(0).value().path(), coll->path()); } - QTRY_COMPARE(spyCollectionDeleted.count(), 0); + VERIFY(waitForSignal(spyCollectionDeleted, 0)); +} + +void TestGuiFdoSecrets::testServiceUnlockDatabaseConcurrent() +{ + lockDatabaseInBackend(); + + auto service = enableService(); + VERIFY(service); + auto coll = getDefaultCollection(service); + VERIFY(coll); + + DBUS_GET2(unlocked, promptPath, service->Unlock({QDBusObjectPath(coll->path())})); + auto prompt = getProxy(promptPath); + VERIFY(prompt); + QSignalSpy spyPromptCompleted(prompt.data(), SIGNAL(Completed(bool, QDBusVariant))); + VERIFY(spyPromptCompleted.isValid()); + DBUS_VERIFY(prompt->Prompt("")); + + // while the first prompt is running, another request come in + DBUS_GET2(unlocked2, promptPath2, service->Unlock({QDBusObjectPath(coll->path())})); + auto prompt2 = getProxy(promptPath2); + VERIFY(prompt2); + QSignalSpy spyPromptCompleted2(prompt2.data(), SIGNAL(Completed(bool, QDBusVariant))); + VERIFY(spyPromptCompleted2.isValid()); + DBUS_VERIFY(prompt2->Prompt("")); + + // there should be only one unlock dialog + VERIFY(driveUnlockDialog()); + + // both prompts should complete + VERIFY(waitForSignal(spyPromptCompleted, 1)); + { + auto args = spyPromptCompleted.takeFirst(); + COMPARE(args.size(), 2); + COMPARE(args.at(0).toBool(), false); + COMPARE(getSignalVariantArgument>(args.at(1)), {QDBusObjectPath(coll->path())}); + } + VERIFY(waitForSignal(spyPromptCompleted2, 1)); + { + auto args = spyPromptCompleted2.takeFirst(); + COMPARE(args.size(), 2); + COMPARE(args.at(0).toBool(), false); + COMPARE(getSignalVariantArgument>(args.at(1)), {QDBusObjectPath(coll->path())}); + } + + // check unlocked *AFTER* prompt signal + DBUS_COMPARE(coll->locked(), false); } void TestGuiFdoSecrets::testServiceUnlockItems() @@ -438,17 +550,16 @@ void TestGuiFdoSecrets::testServiceUnlockItems() // only allow once VERIFY(driveAccessControlDialog(false)); - // unlocked - DBUS_COMPARE(item->locked(), false); - - VERIFY(spyPromptCompleted.wait()); - COMPARE(spyPromptCompleted.count(), 1); + VERIFY(waitForSignal(spyPromptCompleted, 1)); { auto args = spyPromptCompleted.takeFirst(); COMPARE(args.size(), 2); COMPARE(args.at(0).toBool(), false); COMPARE(getSignalVariantArgument>(args.at(1)), {QDBusObjectPath(item->path())}); } + + // unlocked + DBUS_COMPARE(item->locked(), false); } // access the secret should reset the locking state @@ -477,17 +588,16 @@ void TestGuiFdoSecrets::testServiceUnlockItems() // only allow and remember VERIFY(driveAccessControlDialog(true)); - // unlocked - DBUS_COMPARE(item->locked(), false); - - VERIFY(spyPromptCompleted.wait()); - COMPARE(spyPromptCompleted.count(), 1); + VERIFY(waitForSignal(spyPromptCompleted, 1)); { auto args = spyPromptCompleted.takeFirst(); COMPARE(args.size(), 2); COMPARE(args.at(0).toBool(), false); COMPARE(getSignalVariantArgument>(args.at(1)), {QDBusObjectPath(item->path())}); } + + // unlocked + DBUS_COMPARE(item->locked(), false); } // access the secret does not reset the locking state @@ -524,15 +634,15 @@ void TestGuiFdoSecrets::testServiceLock() // prompt and click cancel MessageBox::setNextAnswer(MessageBox::Cancel); DBUS_VERIFY(prompt->Prompt("")); - QApplication::processEvents(); + processEvents(); - DBUS_COMPARE(coll->locked(), false); - - QTRY_COMPARE(spyPromptCompleted.count(), 1); + VERIFY(waitForSignal(spyPromptCompleted, 1)); auto args = spyPromptCompleted.takeFirst(); COMPARE(args.count(), 2); COMPARE(args.at(0).toBool(), true); COMPARE(getSignalVariantArgument>(args.at(1)), {}); + + DBUS_COMPARE(coll->locked(), false); } { DBUS_GET2(locked, promptPath, service->Lock({QDBusObjectPath(coll->path())})); @@ -545,24 +655,24 @@ void TestGuiFdoSecrets::testServiceLock() // prompt and click save MessageBox::setNextAnswer(MessageBox::Save); DBUS_VERIFY(prompt->Prompt("")); - QApplication::processEvents(); + processEvents(); - DBUS_COMPARE(coll->locked(), true); - - QTRY_COMPARE(spyPromptCompleted.count(), 1); + VERIFY(waitForSignal(spyPromptCompleted, 1)); auto args = spyPromptCompleted.takeFirst(); COMPARE(args.count(), 2); COMPARE(args.at(0).toBool(), false); COMPARE(getSignalVariantArgument>(args.at(1)), {QDBusObjectPath(coll->path())}); + + DBUS_COMPARE(coll->locked(), true); } - QTRY_COMPARE(spyCollectionCreated.count(), 0); + VERIFY(waitForSignal(spyCollectionCreated, 0)); QTRY_VERIFY(!spyCollectionChanged.isEmpty()); for (const auto& args : spyCollectionChanged) { COMPARE(args.size(), 1); COMPARE(args.at(0).value().path(), coll->path()); } - QTRY_COMPARE(spyCollectionDeleted.count(), 0); + VERIFY(waitForSignal(spyCollectionDeleted, 0)); // locking item locks the whole db unlockDatabaseInBackend(); @@ -575,12 +685,59 @@ void TestGuiFdoSecrets::testServiceLock() MessageBox::setNextAnswer(MessageBox::Save); DBUS_VERIFY(prompt->Prompt("")); - QApplication::processEvents(); + processEvents(); DBUS_COMPARE(coll->locked(), true); } } +void TestGuiFdoSecrets::testServiceLockConcurrent() +{ + auto service = enableService(); + VERIFY(service); + auto coll = getDefaultCollection(service); + VERIFY(coll); + + m_db->markAsModified(); + + DBUS_GET2(locked, promptPath, service->Lock({QDBusObjectPath(coll->path())})); + auto prompt = getProxy(promptPath); + VERIFY(prompt); + QSignalSpy spyPromptCompleted(prompt.data(), SIGNAL(Completed(bool, QDBusVariant))); + VERIFY(spyPromptCompleted.isValid()); + + DBUS_GET2(locked2, promptPath2, service->Lock({QDBusObjectPath(coll->path())})); + auto prompt2 = getProxy(promptPath2); + VERIFY(prompt2); + QSignalSpy spyPromptCompleted2(prompt2.data(), SIGNAL(Completed(bool, QDBusVariant))); + VERIFY(spyPromptCompleted2.isValid()); + + // prompt and click save + MessageBox::setNextAnswer(MessageBox::Save); + DBUS_VERIFY(prompt->Prompt("")); + + // second prompt should not show dialog + DBUS_VERIFY(prompt2->Prompt("")); + + VERIFY(waitForSignal(spyPromptCompleted, 1)); + { + auto args = spyPromptCompleted.takeFirst(); + COMPARE(args.count(), 2); + COMPARE(args.at(0).toBool(), false); + COMPARE(getSignalVariantArgument>(args.at(1)), {QDBusObjectPath(coll->path())}); + } + + VERIFY(waitForSignal(spyPromptCompleted2, 1)); + { + auto args = spyPromptCompleted2.takeFirst(); + COMPARE(args.count(), 2); + COMPARE(args.at(0).toBool(), false); + COMPARE(getSignalVariantArgument>(args.at(1)), {QDBusObjectPath(coll->path())}); + } + + DBUS_COMPARE(coll->locked(), true); +} + void TestGuiFdoSecrets::testSessionOpen() { auto service = enableService(); @@ -621,7 +778,7 @@ void TestGuiFdoSecrets::testCollectionCreate() COMPARE(promptPath, QDBusObjectPath("/")); COMPARE(collPath.path(), existing->path()); } - QTRY_COMPARE(spyCollectionCreated.count(), 0); + VERIFY(waitForSignal(spyCollectionCreated, 0)); // create new one and set properties { @@ -635,11 +792,10 @@ void TestGuiFdoSecrets::testCollectionCreate() QSignalSpy spyPromptCompleted(prompt.data(), SIGNAL(Completed(bool, QDBusVariant))); VERIFY(spyPromptCompleted.isValid()); - QTimer::singleShot(50, this, &TestGuiFdoSecrets::driveNewDatabaseWizard); DBUS_VERIFY(prompt->Prompt("")); - QApplication::processEvents(); + VERIFY(driveNewDatabaseWizard()); - QTRY_COMPARE(spyPromptCompleted.count(), 1); + VERIFY(waitForSignal(spyPromptCompleted, 1)); auto args = spyPromptCompleted.takeFirst(); COMPARE(args.size(), 2); COMPARE(args.at(0).toBool(), false); @@ -648,7 +804,7 @@ void TestGuiFdoSecrets::testCollectionCreate() DBUS_COMPARE(coll->label(), QStringLiteral("Test NewDB")); - QTRY_COMPARE(spyCollectionCreated.count(), 1); + VERIFY(waitForSignal(spyCollectionCreated, 1)); { args = spyCollectionCreated.takeFirst(); COMPARE(args.size(), 1); @@ -657,34 +813,6 @@ void TestGuiFdoSecrets::testCollectionCreate() } } -void TestGuiFdoSecrets::driveNewDatabaseWizard() -{ - auto wizard = m_tabWidget->findChild(); - VERIFY(wizard); - - COMPARE(wizard->currentId(), 0); - wizard->next(); - wizard->next(); - COMPARE(wizard->currentId(), 2); - - // enter password - auto* passwordEdit = wizard->findChild("enterPasswordEdit"); - auto* passwordRepeatEdit = wizard->findChild("repeatPasswordEdit"); - QTest::keyClicks(passwordEdit, "test"); - QTest::keyClick(passwordEdit, Qt::Key::Key_Tab); - QTest::keyClicks(passwordRepeatEdit, "test"); - - // save database to temporary file - TemporaryFile tmpFile; - VERIFY(tmpFile.open()); - tmpFile.close(); - fileDialog()->setNextFileName(tmpFile.fileName()); - - wizard->accept(); - - tmpFile.remove(); -} - void TestGuiFdoSecrets::testCollectionDelete() { auto service = enableService(); @@ -711,7 +839,12 @@ void TestGuiFdoSecrets::testCollectionDelete() // closing the tab should have deleted the database if not in testing // but deleteLater is not processed in QApplication::processEvent // see https://doc.qt.io/qt-5/qcoreapplication.html#processEvents - QApplication::processEvents(); + + VERIFY(waitForSignal(spyPromptCompleted, 1)); + auto args = spyPromptCompleted.takeFirst(); + COMPARE(args.count(), 2); + COMPARE(args.at(0).toBool(), false); + COMPARE(args.at(1).value().variant().toString(), QStringLiteral("")); // however, the object should already be taken down from dbus { @@ -720,13 +853,7 @@ void TestGuiFdoSecrets::testCollectionDelete() COMPARE(reply.error().type(), QDBusError::UnknownObject); } - QTRY_COMPARE(spyPromptCompleted.count(), 1); - auto args = spyPromptCompleted.takeFirst(); - COMPARE(args.count(), 2); - COMPARE(args.at(0).toBool(), false); - COMPARE(args.at(1).value().variant().toString(), QStringLiteral("")); - - QTRY_COMPARE(spyCollectionDeleted.count(), 1); + VERIFY(waitForSignal(spyCollectionDeleted, 1)); { args = spyCollectionDeleted.takeFirst(); COMPARE(args.size(), 1); @@ -734,6 +861,57 @@ void TestGuiFdoSecrets::testCollectionDelete() } } +void TestGuiFdoSecrets::testCollectionDeleteConcurrent() +{ + auto service = enableService(); + VERIFY(service); + auto coll = getDefaultCollection(service); + VERIFY(coll); + + m_db->markAsModified(); + DBUS_GET(promptPath, coll->Delete()); + auto prompt = getProxy(promptPath); + VERIFY(prompt); + QSignalSpy spyPromptCompleted(prompt.data(), SIGNAL(Completed(bool, QDBusVariant))); + VERIFY(spyPromptCompleted.isValid()); + + // before interacting with the prompt, another request come in + DBUS_GET(promptPath2, coll->Delete()); + auto prompt2 = getProxy(promptPath); + VERIFY(prompt2); + QSignalSpy spyPromptCompleted2(prompt2.data(), SIGNAL(Completed(bool, QDBusVariant))); + VERIFY(spyPromptCompleted2.isValid()); + + // prompt and click save + MessageBox::setNextAnswer(MessageBox::Save); + DBUS_VERIFY(prompt->Prompt("")); + + // there should be no prompt + DBUS_VERIFY(prompt2->Prompt("")); + + VERIFY(waitForSignal(spyPromptCompleted, 1)); + { + auto args = spyPromptCompleted.takeFirst(); + COMPARE(args.count(), 2); + COMPARE(args.at(0).toBool(), false); + COMPARE(args.at(1).value().variant().toString(), QStringLiteral("")); + } + + VERIFY(waitForSignal(spyPromptCompleted2, 1)); + { + auto args = spyPromptCompleted2.takeFirst(); + COMPARE(args.count(), 2); + COMPARE(args.at(0).toBool(), false); + COMPARE(args.at(1).value().variant().toString(), QStringLiteral("")); + } + + { + auto reply = coll->locked(); + VERIFY(reply.isFinished() && reply.isError()); + COMPARE(reply.error().type(), QDBusError::UnknownObject); + } +} + void TestGuiFdoSecrets::testCollectionChange() { auto service = enableService(); @@ -822,7 +1000,7 @@ void TestGuiFdoSecrets::testItemCreate() // signals { - QTRY_COMPARE(spyItemCreated.count(), 1); + VERIFY(waitForSignal(spyItemCreated, 1)); auto args = spyItemCreated.takeFirst(); COMPARE(args.size(), 1); COMPARE(args.at(0).value().path(), item->path()); @@ -942,7 +1120,7 @@ void TestGuiFdoSecrets::testItemReplace() QSet expected{QDBusObjectPath(item1->path()), QDBusObjectPath(item2->path())}; COMPARE(QSet::fromList(unlocked), expected); - QTRY_COMPARE(spyItemCreated.count(), 0); + VERIFY(waitForSignal(spyItemCreated, 0)); // there may be multiple changed signals, due to each item attribute is set separately QTRY_VERIFY(!spyItemChanged.isEmpty()); for (const auto& args : spyItemChanged) { @@ -968,7 +1146,7 @@ void TestGuiFdoSecrets::testItemReplace() }; COMPARE(QSet::fromList(unlocked), expected); - QTRY_COMPARE(spyItemCreated.count(), 1); + VERIFY(waitForSignal(spyItemCreated, 1)); { auto args = spyItemCreated.takeFirst(); COMPARE(args.size(), 1); @@ -1013,7 +1191,7 @@ void TestGuiFdoSecrets::testItemReplaceExistingLocked() DBUS_COMPARE(item->locked(), true); } - // when replace with a locked item, there will be an prompt + // when replace with a locked item, there will be a prompt auto item2 = createItem(sess, coll, "abc2", "PasswordUpdated", attr1, true, true); VERIFY(item2); COMPARE(item2->path(), item->path()); @@ -1061,7 +1239,7 @@ void TestGuiFdoSecrets::testItemSecret() COMPARE(ss.contentType, TEXT_PLAIN); COMPARE(ss.value, entry->password().toUtf8()); - QTRY_COMPARE(spyShowNotification.count(), 1); + VERIFY(waitForSignal(spyShowNotification, 1)); } FdoSecrets::settings()->setShowNotification(false); @@ -1125,15 +1303,14 @@ void TestGuiFdoSecrets::testItemDelete() VERIFY(itemObj); MessageBox::setNextAnswer(MessageBox::Delete); DBUS_VERIFY(prompt->Prompt("")); - QApplication::processEvents(); - QTRY_COMPARE(spyPromptCompleted.count(), 1); + VERIFY(waitForSignal(spyPromptCompleted, 1)); auto args = spyPromptCompleted.takeFirst(); COMPARE(args.count(), 2); COMPARE(args.at(0).toBool(), false); COMPARE(args.at(1).toString(), QStringLiteral("")); - QTRY_COMPARE(spyItemDeleted.count(), 1); + VERIFY(waitForSignal(spyItemDeleted, 1)); args = spyItemDeleted.takeFirst(); COMPARE(args.size(), 1); COMPARE(args.at(0).value().path(), itemPath); @@ -1275,7 +1452,7 @@ void TestGuiFdoSecrets::testModifyingExposedGroup() m_db->metadata()->setRecycleBinEnabled(true); m_db->recycleGroup(subgroup); - QApplication::processEvents(); + processEvents(); { DBUS_GET(collPaths, service->collections()); @@ -1284,7 +1461,7 @@ void TestGuiFdoSecrets::testModifyingExposedGroup() // test setting another exposed group, the collection will be exposed again FdoSecrets::settings()->setExposedGroup(m_db, m_db->rootGroup()->uuid()); - QApplication::processEvents(); + processEvents(); { DBUS_GET(collPaths, service->collections()); COMPARE(collPaths.size(), 1); @@ -1295,14 +1472,25 @@ void TestGuiFdoSecrets::lockDatabaseInBackend() { m_dbWidget->lock(); m_db.reset(); - QApplication::processEvents(); + processEvents(); } void TestGuiFdoSecrets::unlockDatabaseInBackend() { m_dbWidget->performUnlockDatabase("a"); m_db = m_dbWidget->database(); - QApplication::processEvents(); + processEvents(); +} + +void TestGuiFdoSecrets::processEvents() +{ + // Couldn't use QApplication::processEvents, because per Qt documentation: + // events that are posted while the function runs will be queued until a later round of event processing. + // and we may post QTimer single shot events during event handling to achieve async method. + // So we directly call event dispatcher in a loop until no events can be handled + while (QAbstractEventDispatcher::instance()->processEvents(QEventLoop::AllEvents)) { + // pass + } } // the following functions have return value, switch macros to the version supporting that @@ -1388,9 +1576,7 @@ QSharedPointer TestGuiFdoSecrets::createItem(const QSharedPointer TestGuiFdoSecrets::createItem(const QSharedPointerallWidgets()) { + processEvents(); + for (auto w : QApplication::topLevelWidgets()) { if (!w->isWindow()) { continue; } auto dlg = qobject_cast(w); - if (dlg) { + if (dlg && dlg->isVisible()) { auto rememberCheck = dlg->findChild("rememberCheck"); VERIFY(rememberCheck); rememberCheck->setChecked(remember); - QTest::keyClick(dlg, Qt::Key_Enter); - QApplication::processEvents(); + dlg->done(AccessControlDialog::AllowSelected); + processEvents(); + VERIFY(dlg->isHidden()); return true; } } return false; } +bool TestGuiFdoSecrets::driveNewDatabaseWizard() +{ + // processEvents will block because the NewDatabaseWizard is shown using exec + // which creates a local QEventLoop. + // so we do a little trick here to get the return value back + bool ret = false; + QTimer::singleShot(0, this, [this, &ret]() { + ret = [this]() -> bool { + auto wizard = m_tabWidget->findChild(); + VERIFY(wizard); + + COMPARE(wizard->currentId(), 0); + wizard->next(); + wizard->next(); + COMPARE(wizard->currentId(), 2); + + // enter password + auto* passwordEdit = wizard->findChild("enterPasswordEdit"); + auto* passwordRepeatEdit = wizard->findChild("repeatPasswordEdit"); + VERIFY(passwordEdit); + VERIFY(passwordRepeatEdit); + QTest::keyClicks(passwordEdit, "test"); + QTest::keyClick(passwordEdit, Qt::Key::Key_Tab); + QTest::keyClicks(passwordRepeatEdit, "test"); + + // save database to temporary file + TemporaryFile tmpFile; + VERIFY(tmpFile.open()); + tmpFile.close(); + fileDialog()->setNextFileName(tmpFile.fileName()); + + wizard->accept(); + + tmpFile.remove(); + return true; + }(); + }); + processEvents(); + return ret; +} + +bool TestGuiFdoSecrets::driveUnlockDialog() +{ + processEvents(); + auto dbOpenDlg = m_tabWidget->findChild(); + VERIFY(dbOpenDlg); + auto editPassword = dbOpenDlg->findChild("editPassword"); + VERIFY(editPassword); + editPassword->setFocus(); + QTest::keyClicks(editPassword, "a"); + QTest::keyClick(editPassword, Qt::Key_Enter); + processEvents(); + return true; +} + +bool TestGuiFdoSecrets::waitForSignal(QSignalSpy& spy, int expectedCount) +{ + processEvents(); + // If already expected count, do not wait and return immediately + if (spy.count() == expectedCount) { + return true; + } else if (spy.count() > expectedCount) { + return false; + } + spy.wait(); + COMPARE(spy.count(), expectedCount); + return true; +} + #undef VERIFY #define VERIFY QVERIFY #undef COMPARE diff --git a/tests/gui/TestGuiFdoSecrets.h b/tests/gui/TestGuiFdoSecrets.h index a53da4088..1ed6d7f66 100644 --- a/tests/gui/TestGuiFdoSecrets.h +++ b/tests/gui/TestGuiFdoSecrets.h @@ -23,6 +23,7 @@ #include #include "fdosecrets/dbus/DBusTypes.h" +#include "gui/MessageBox.h" class MainWindow; class Database; @@ -47,6 +48,7 @@ class SessionProxy; class PromptProxy; class QAbstractItemView; +class QSignalSpy; class TestGuiFdoSecrets : public QObject { @@ -64,15 +66,20 @@ private slots: void testServiceEnable(); void testServiceEnableNoExposedDatabase(); void testServiceSearch(); + void testServiceSearchBlockingUnlock(); + void testServiceSearchForce(); void testServiceUnlock(); + void testServiceUnlockDatabaseConcurrent(); void testServiceUnlockItems(); void testServiceLock(); + void testServiceLockConcurrent(); void testSessionOpen(); void testSessionClose(); void testCollectionCreate(); void testCollectionDelete(); + void testCollectionDeleteConcurrent(); void testCollectionChange(); void testItemCreate(); @@ -92,11 +99,14 @@ private slots: void testHiddenFilename(); void testDuplicateName(); -protected slots: - void driveNewDatabaseWizard(); - bool driveAccessControlDialog(bool remember = true); - private: + bool driveUnlockDialog(); + bool driveNewDatabaseWizard(); + bool driveAccessControlDialog(bool remember = true); + bool waitForSignal(QSignalSpy& spy, int expectedCount); + + void processEvents(); + void lockDatabaseInBackend(); void unlockDatabaseInBackend(); QSharedPointer enableService();