From 9f4118974dca3335b683e79b172fd94b48afdeda Mon Sep 17 00:00:00 2001 From: Aetf Date: Fri, 13 Nov 2020 17:14:03 -0500 Subject: [PATCH] FdoSecrets: fix signal connections --- .../objects/adaptors/CollectionAdaptor.cpp | 16 ++- src/fdosecrets/objects/adaptors/DBusAdaptor.h | 2 +- .../objects/adaptors/PromptAdaptor.cpp | 3 +- .../objects/adaptors/ServiceAdaptor.cpp | 7 +- tests/gui/TestGuiFdoSecrets.cpp | 109 +++++++++++------- 5 files changed, 87 insertions(+), 50 deletions(-) diff --git a/src/fdosecrets/objects/adaptors/CollectionAdaptor.cpp b/src/fdosecrets/objects/adaptors/CollectionAdaptor.cpp index 77eb3b413f..275145b440 100644 --- a/src/fdosecrets/objects/adaptors/CollectionAdaptor.cpp +++ b/src/fdosecrets/objects/adaptors/CollectionAdaptor.cpp @@ -27,12 +27,16 @@ namespace FdoSecrets CollectionAdaptor::CollectionAdaptor(Collection* parent) : DBusAdaptor(parent) { - connect( - p(), &Collection::itemCreated, this, [this](const Item* item) { emit ItemCreated(objectPathSafe(item)); }); - connect( - p(), &Collection::itemDeleted, this, [this](const Item* item) { emit ItemDeleted(objectPathSafe(item)); }); - connect( - p(), &Collection::itemChanged, this, [this](const Item* item) { emit ItemChanged(objectPathSafe(item)); }); + // p() isn't ready yet as this is called in Parent's constructor + connect(parent, &Collection::itemCreated, this, [this](const Item* item) { + emit ItemCreated(objectPathSafe(item)); + }); + connect(parent, &Collection::itemDeleted, this, [this](const Item* item) { + emit ItemDeleted(objectPathSafe(item)); + }); + connect(parent, &Collection::itemChanged, this, [this](const Item* item) { + emit ItemChanged(objectPathSafe(item)); + }); } const QList CollectionAdaptor::items() const diff --git a/src/fdosecrets/objects/adaptors/DBusAdaptor.h b/src/fdosecrets/objects/adaptors/DBusAdaptor.h index bd70ab60a3..93bbc72f0c 100644 --- a/src/fdosecrets/objects/adaptors/DBusAdaptor.h +++ b/src/fdosecrets/objects/adaptors/DBusAdaptor.h @@ -32,7 +32,7 @@ namespace FdoSecrets template class DBusAdaptor : public QDBusAbstractAdaptor { public: - explicit DBusAdaptor(QObject* parent = nullptr) + explicit DBusAdaptor(Parent* parent = nullptr) : QDBusAbstractAdaptor(parent) { } diff --git a/src/fdosecrets/objects/adaptors/PromptAdaptor.cpp b/src/fdosecrets/objects/adaptors/PromptAdaptor.cpp index bcc1e271dd..ff8a945cda 100644 --- a/src/fdosecrets/objects/adaptors/PromptAdaptor.cpp +++ b/src/fdosecrets/objects/adaptors/PromptAdaptor.cpp @@ -25,7 +25,8 @@ namespace FdoSecrets PromptAdaptor::PromptAdaptor(PromptBase* parent) : DBusAdaptor(parent) { - connect(p(), &PromptBase::completed, this, [this](bool dismissed, QVariant result) { + // p() isn't ready yet as this is called in Parent's constructor + connect(parent, &PromptBase::completed, this, [this](bool dismissed, QVariant result) { // make sure the result contains a valid value, otherwise QDBusVariant refuses to marshall it. if (!result.isValid()) { result = QString{}; diff --git a/src/fdosecrets/objects/adaptors/ServiceAdaptor.cpp b/src/fdosecrets/objects/adaptors/ServiceAdaptor.cpp index a260c4920d..cacf9a994b 100644 --- a/src/fdosecrets/objects/adaptors/ServiceAdaptor.cpp +++ b/src/fdosecrets/objects/adaptors/ServiceAdaptor.cpp @@ -29,13 +29,14 @@ namespace FdoSecrets ServiceAdaptor::ServiceAdaptor(Service* parent) : DBusAdaptor(parent) { - connect(p(), &Service::collectionCreated, this, [this](Collection* coll) { + // p() isn't ready yet as this is called in Parent's constructor + connect(parent, &Service::collectionCreated, this, [this](Collection* coll) { emit CollectionCreated(objectPathSafe(coll)); }); - connect(p(), &Service::collectionDeleted, this, [this](Collection* coll) { + connect(parent, &Service::collectionDeleted, this, [this](Collection* coll) { emit CollectionDeleted(objectPathSafe(coll)); }); - connect(p(), &Service::collectionChanged, this, [this](Collection* coll) { + connect(parent, &Service::collectionChanged, this, [this](Collection* coll) { emit CollectionChanged(objectPathSafe(coll)); }); } diff --git a/tests/gui/TestGuiFdoSecrets.cpp b/tests/gui/TestGuiFdoSecrets.cpp index 08840088f6..eb8192d5f0 100644 --- a/tests/gui/TestGuiFdoSecrets.cpp +++ b/tests/gui/TestGuiFdoSecrets.cpp @@ -174,18 +174,10 @@ namespace using namespace FdoSecrets; -// for use in QSignalSpy -Q_DECLARE_METATYPE(Collection*); -Q_DECLARE_METATYPE(Item*); - TestGuiFdoSecrets::~TestGuiFdoSecrets() = default; void TestGuiFdoSecrets::initTestCase() { - // for use in QSignalSpy - qRegisterMetaType(); - qRegisterMetaType(); - QVERIFY(Crypto::init()); Config::createTempFileInstance(); config()->set(Config::AutoSaveAfterEveryChange, false); @@ -458,11 +450,11 @@ void TestGuiFdoSecrets::testServiceUnlock() auto coll = getDefaultCollection(service); QVERIFY(coll); - QSignalSpy spyCollectionCreated(service, SIGNAL(collectionCreated(Collection*))); + QSignalSpy spyCollectionCreated(&service->dbusAdaptor(), SIGNAL(CollectionCreated(QDBusObjectPath))); QVERIFY(spyCollectionCreated.isValid()); - QSignalSpy spyCollectionDeleted(service, SIGNAL(collectionDeleted(Collection*))); + QSignalSpy spyCollectionDeleted(&service->dbusAdaptor(), SIGNAL(CollectionDeleted(QDBusObjectPath))); QVERIFY(spyCollectionDeleted.isValid()); - QSignalSpy spyCollectionChanged(service, SIGNAL(collectionChanged(Collection*))); + QSignalSpy spyCollectionChanged(&service->dbusAdaptor(), SIGNAL(CollectionChanged(QDBusObjectPath))); QVERIFY(spyCollectionChanged.isValid()); PromptBase* prompt = nullptr; @@ -472,7 +464,7 @@ void TestGuiFdoSecrets::testServiceUnlock() QVERIFY(unlocked.isEmpty()); } QVERIFY(prompt); - QSignalSpy spyPromptCompleted(prompt, SIGNAL(completed(bool, QVariant))); + QSignalSpy spyPromptCompleted(&prompt->dbusAdaptor(), SIGNAL(Completed(bool, QDBusVariant))); QVERIFY(spyPromptCompleted.isValid()); // nothing is unlocked yet @@ -510,14 +502,14 @@ void TestGuiFdoSecrets::testServiceUnlock() auto args = spyPromptCompleted.takeFirst(); QCOMPARE(args.size(), 2); QCOMPARE(args.at(0).toBool(), false); - QCOMPARE(args.at(1).value>(), {coll->objectPath()}); + QCOMPARE(args.at(1).value().variant().value>(), {coll->objectPath()}); } QCOMPARE(spyCollectionCreated.count(), 0); QCOMPARE(spyCollectionChanged.count(), 1); { auto args = spyCollectionChanged.takeFirst(); QCOMPARE(args.size(), 1); - QCOMPARE(args.at(0).value(), coll.data()); + QCOMPARE(args.at(0).value(), coll->objectPath()); } QCOMPARE(spyCollectionDeleted.count(), 0); } @@ -529,11 +521,11 @@ void TestGuiFdoSecrets::testServiceLock() auto coll = getDefaultCollection(service); QVERIFY(coll); - QSignalSpy spyCollectionCreated(service, SIGNAL(collectionCreated(Collection*))); + QSignalSpy spyCollectionCreated(&service->dbusAdaptor(), SIGNAL(CollectionCreated(QDBusObjectPath))); QVERIFY(spyCollectionCreated.isValid()); - QSignalSpy spyCollectionDeleted(service, SIGNAL(collectionDeleted(Collection*))); + QSignalSpy spyCollectionDeleted(&service->dbusAdaptor(), SIGNAL(CollectionDeleted(QDBusObjectPath))); QVERIFY(spyCollectionDeleted.isValid()); - QSignalSpy spyCollectionChanged(service, SIGNAL(collectionChanged(Collection*))); + QSignalSpy spyCollectionChanged(&service->dbusAdaptor(), SIGNAL(CollectionChanged(QDBusObjectPath))); QVERIFY(spyCollectionChanged.isValid()); // if the db is modified, prompt user @@ -543,7 +535,7 @@ void TestGuiFdoSecrets::testServiceLock() CHECKED_DBUS_LOCAL_CALL(locked, service->lock({coll}, prompt)); QCOMPARE(locked.size(), 0); QVERIFY(prompt); - QSignalSpy spyPromptCompleted(prompt, SIGNAL(completed(bool, QVariant))); + QSignalSpy spyPromptCompleted(&prompt->dbusAdaptor(), SIGNAL(Completed(bool, QDBusVariant))); QVERIFY(spyPromptCompleted.isValid()); // prompt and click cancel @@ -564,7 +556,7 @@ void TestGuiFdoSecrets::testServiceLock() CHECKED_DBUS_LOCAL_CALL(locked, service->lock({coll}, prompt)); QCOMPARE(locked.size(), 0); QVERIFY(prompt); - QSignalSpy spyPromptCompleted(prompt, SIGNAL(completed(bool, QVariant))); + QSignalSpy spyPromptCompleted(&prompt->dbusAdaptor(), SIGNAL(Completed(bool, QDBusVariant))); QVERIFY(spyPromptCompleted.isValid()); // prompt and click save @@ -578,7 +570,7 @@ void TestGuiFdoSecrets::testServiceLock() auto args = spyPromptCompleted.takeFirst(); QCOMPARE(args.count(), 2); QCOMPARE(args.at(0).toBool(), false); - QCOMPARE(args.at(1).value>(), {coll->objectPath()}); + QCOMPARE(args.at(1).value().variant().value>(), {coll->objectPath()}); } QCOMPARE(spyCollectionCreated.count(), 0); @@ -586,7 +578,7 @@ void TestGuiFdoSecrets::testServiceLock() { auto args = spyCollectionChanged.takeFirst(); QCOMPARE(args.size(), 1); - QCOMPARE(args.at(0).value(), coll.data()); + QCOMPARE(args.at(0).value(), coll->objectPath()); } QCOMPARE(spyCollectionDeleted.count(), 0); @@ -642,7 +634,7 @@ void TestGuiFdoSecrets::testCollectionCreate() auto service = enableService(); QVERIFY(service); - QSignalSpy spyCollectionCreated(service, SIGNAL(collectionCreated(Collection*))); + QSignalSpy spyCollectionCreated(&service->dbusAdaptor(), SIGNAL(CollectionCreated(QDBusObjectPath))); QVERIFY(spyCollectionCreated.isValid()); // returns existing if alias is nonempty and exists @@ -664,7 +656,7 @@ void TestGuiFdoSecrets::testCollectionCreate() QVERIFY(!created); QVERIFY(prompt); - QSignalSpy spyPromptCompleted(prompt, SIGNAL(completed(bool, QVariant))); + QSignalSpy spyPromptCompleted(&prompt->dbusAdaptor(), SIGNAL(Completed(bool, QDBusVariant))); QVERIFY(spyPromptCompleted.isValid()); QTimer::singleShot(50, this, SLOT(createDatabaseCallback())); @@ -675,7 +667,8 @@ void TestGuiFdoSecrets::testCollectionCreate() auto args = spyPromptCompleted.takeFirst(); QCOMPARE(args.size(), 2); QCOMPARE(args.at(0).toBool(), false); - auto coll = FdoSecrets::pathToObject(args.at(1).value()); + auto coll = + FdoSecrets::pathToObject(args.at(1).value().variant().value()); QVERIFY(coll); QCOMPARE(coll->backend()->database()->metadata()->name(), QStringLiteral("Test NewDB")); @@ -684,7 +677,7 @@ void TestGuiFdoSecrets::testCollectionCreate() { args = spyCollectionCreated.takeFirst(); QCOMPARE(args.size(), 1); - QCOMPARE(args.at(0).value(), coll); + QCOMPARE(args.at(0).value(), coll->objectPath()); } } } @@ -723,18 +716,16 @@ void TestGuiFdoSecrets::testCollectionDelete() QVERIFY(service); auto coll = getDefaultCollection(service); QVERIFY(coll); - // closing the tab calls coll->deleteLater() - // but deleteLater is not processed in QApplication::processEvent - // see https://doc.qt.io/qt-5/qcoreapplication.html#processEvents - auto rawColl = coll.data(); + // save the path which will be gone after the deletion. + auto collPath = coll->objectPath(); - QSignalSpy spyCollectionDeleted(service, SIGNAL(collectionDeleted(Collection*))); + QSignalSpy spyCollectionDeleted(&service->dbusAdaptor(), SIGNAL(CollectionDeleted(QDBusObjectPath))); QVERIFY(spyCollectionDeleted.isValid()); m_db->markAsModified(); CHECKED_DBUS_LOCAL_CALL(prompt, coll->deleteCollection()); QVERIFY(prompt); - QSignalSpy spyPromptCompleted(prompt, SIGNAL(completed(bool, QVariant))); + QSignalSpy spyPromptCompleted(&prompt->dbusAdaptor(), SIGNAL(Completed(bool, QDBusVariant))); QVERIFY(spyPromptCompleted.isValid()); // prompt and click save @@ -752,13 +743,13 @@ void TestGuiFdoSecrets::testCollectionDelete() auto args = spyPromptCompleted.takeFirst(); QCOMPARE(args.count(), 2); QCOMPARE(args.at(0).toBool(), false); - QCOMPARE(args.at(1).toString(), QStringLiteral("")); + QCOMPARE(args.at(1).value().variant().toString(), QStringLiteral("")); QCOMPARE(spyCollectionDeleted.count(), 1); { args = spyCollectionDeleted.takeFirst(); QCOMPARE(args.size(), 1); - QCOMPARE(args.at(0).value(), rawColl); + QCOMPARE(args.at(0).value(), collPath); } } @@ -815,6 +806,9 @@ void TestGuiFdoSecrets::testItemCreate() auto sess = openSession(service, DhIetf1024Sha256Aes128CbcPkcs7::Algorithm); QVERIFY(sess); + QSignalSpy spyItemCreated(&coll->dbusAdaptor(), SIGNAL(ItemCreated(QDBusObjectPath))); + QVERIFY(spyItemCreated.isValid()); + // create item StringStringMap attributes{ {"application", "fdosecrets-test"}, @@ -824,6 +818,14 @@ void TestGuiFdoSecrets::testItemCreate() auto item = createItem(sess, coll, "abc", "Password", attributes, false); QVERIFY(item); + // signals + { + QCOMPARE(spyItemCreated.count(), 1); + auto args = spyItemCreated.takeFirst(); + QCOMPARE(args.size(), 1); + QCOMPARE(args.at(0).value(), item->objectPath()); + } + // attributes { CHECKED_DBUS_LOCAL_CALL(actual, item->attributes()); @@ -888,28 +890,56 @@ void TestGuiFdoSecrets::testItemReplace() QCOMPARE(unlocked.size(), 2); } + QSignalSpy spyItemCreated(&coll->dbusAdaptor(), SIGNAL(ItemCreated(QDBusObjectPath))); + QVERIFY(spyItemCreated.isValid()); + QSignalSpy spyItemChanged(&coll->dbusAdaptor(), SIGNAL(ItemChanged(QDBusObjectPath))); + QVERIFY(spyItemChanged.isValid()); + { // when replace, existing item with matching attr is updated auto item3 = createItem(sess, coll, "abc3", "Password", attr2, true); QVERIFY(item3); QCOMPARE(item2, item3); COMPARE_DBUS_LOCAL_CALL(item3->label(), QStringLiteral("abc3")); - // there is still 2 entries + // there are still 2 entries QList locked; CHECKED_DBUS_LOCAL_CALL(unlocked, service->searchItems({{"application", "fdosecrets-test"}}, locked)); QCOMPARE(unlocked.size(), 2); + + QCOMPARE(spyItemCreated.count(), 0); + // there may be multiple changed signals, due to each item attribute is set separately + QVERIFY(!spyItemChanged.isEmpty()); + for (const auto& args : spyItemChanged) { + QCOMPARE(args.size(), 1); + QCOMPARE(args.at(0).value(), item3->objectPath()); + } } + spyItemCreated.clear(); + spyItemChanged.clear(); { // when NOT replace, another entry is created auto item4 = createItem(sess, coll, "abc4", "Password", attr2, false); QVERIFY(item4); COMPARE_DBUS_LOCAL_CALL(item2->label(), QStringLiteral("abc3")); COMPARE_DBUS_LOCAL_CALL(item4->label(), QStringLiteral("abc4")); - // there is 3 entries + // there are 3 entries QList locked; CHECKED_DBUS_LOCAL_CALL(unlocked, service->searchItems({{"application", "fdosecrets-test"}}, locked)); QCOMPARE(unlocked.size(), 3); + + QCOMPARE(spyItemCreated.count(), 1); + { + auto args = spyItemCreated.takeFirst(); + QCOMPARE(args.size(), 1); + QCOMPARE(args.at(0).value(), item4->objectPath()); + } + // there may be multiple changed signals, due to each item attribute is set separately + QVERIFY(!spyItemChanged.isEmpty()); + for (const auto& args : spyItemChanged) { + QCOMPARE(args.size(), 1); + QCOMPARE(args.at(0).value(), item4->objectPath()); + } } } @@ -994,15 +1024,16 @@ void TestGuiFdoSecrets::testItemDelete() QVERIFY(coll); auto item = getFirstItem(coll); QVERIFY(item); - auto rawItem = item.data(); + // save the path which will be gone after the deletion. + auto itemPath = item->objectPath(); - QSignalSpy spyItemDeleted(coll, SIGNAL(itemDeleted(Item*))); + QSignalSpy spyItemDeleted(&coll->dbusAdaptor(), SIGNAL(ItemDeleted(QDBusObjectPath))); QVERIFY(spyItemDeleted.isValid()); CHECKED_DBUS_LOCAL_CALL(prompt, item->deleteItem()); QVERIFY(prompt); - QSignalSpy spyPromptCompleted(prompt, SIGNAL(completed(bool, QVariant))); + QSignalSpy spyPromptCompleted(&prompt->dbusAdaptor(), SIGNAL(Completed(bool, QDBusVariant))); QVERIFY(spyPromptCompleted.isValid()); // prompt and click save @@ -1025,7 +1056,7 @@ void TestGuiFdoSecrets::testItemDelete() { args = spyItemDeleted.takeFirst(); QCOMPARE(args.size(), 1); - QCOMPARE(args.at(0).value(), rawItem); + QCOMPARE(args.at(0).value(), itemPath); } }