FdoSecrets: ask to unlock the database when creating items

Also only emit databaseUnlockFinished after the database is unlocked

Fix #7989

src/fdosecrets/objects/Prompt.cpp

@@ -210,7 +210,6 @@ namespace FdoSecrets
         m_collections.reserve(colls.size());
         for (const auto& coll : asConst(colls)) {
             m_collections << coll;
-            connect(coll, &Collection::doneUnlockCollection, this, &UnlockPrompt::collectionUnlockFinished);
         }
         for (const auto& item : asConst(items)) {
             m_items[item->collection()] << item;
@@ -234,6 +233,7 @@ namespace FdoSecrets
         bool waitingForCollections = false;
         for (const auto& c : asConst(m_collections)) {
             if (c) {
+                connect(c, &Collection::doneUnlockCollection, this, &UnlockPrompt::collectionUnlockFinished);
                 // 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();
@@ -242,7 +242,7 @@ namespace FdoSecrets
         }
 
         // unlock items directly if no collection unlocking pending
-        // o.w. do it in collectionUnlockFinished
+        // o.w. doing it in collectionUnlockFinished
         if (!waitingForCollections) {
             unlockItems();
         }
@@ -400,18 +400,49 @@ namespace FdoSecrets
             return PromptResult::accepted(false);
         }
 
-        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;
 
+        // give the user a chance to unlock the collection
+        // UnlockPrompt will handle the case of collection already unlocked
+        auto prompt = PromptBase::Create<UnlockPrompt>(service(), QSet<Collection*>{m_coll.data()}, QSet<Item*>{});
+        if (!prompt) {
+            return DBusResult{QDBusError::InternalError};
+        }
+        // postpone anything after the prompt
+        connect(prompt, &PromptBase::completed, this, [this, windowId](bool dismissed) {
+            if (dismissed) {
+                finishPrompt(dismissed);
+            } else {
+                auto res = createItem(windowId);
+                if (res.err()) {
+                    qWarning() << "FdoSecrets:" << res;
+                    finishPrompt(true);
+                }
+            }
+        });
+
+        auto ret = prompt->prompt(client, windowId);
+        if (ret.err()) {
+            return ret;
+        }
+        return PromptResult::Pending;
+    }
+
+    DBusResult CreateItemPrompt::createItem(const QString& windowId)
+    {
+        auto client = m_client.lock();
+        if (!client) {
+            // client already gone
+            return {};
+        }
+
+        if (!m_coll) {
+            return DBusResult{DBUS_ERROR_SECRET_NO_SUCH_OBJECT};
+        }
+
         // get itemPath to create item and
-        // try finding an existing item using attributes
+        // try to find an existing item using attributes
         QString itemPath{};
         auto iterAttr = m_properties.find(DBUS_INTERFACE_SECRET_ITEM + ".Attributes");
         if (iterAttr != m_properties.end()) {
@@ -425,7 +456,7 @@ namespace FdoSecrets
 
             // check existing item using attributes
             QList<Item*> existing;
-            ret = m_coll->searchItems(client, attributes, existing);
+            auto ret = m_coll->searchItems(client, attributes, existing);
             if (ret.err()) {
                 return ret;
             }
@@ -444,31 +475,29 @@ namespace FdoSecrets
         }
 
         // the item may be locked due to authorization
-        ret = m_item->locked(client, locked);
+        // give the user a chance to unlock the item
+        auto prompt = PromptBase::Create<UnlockPrompt>(service(), QSet<Collection*>{}, QSet<Item*>{m_item});
+        if (!prompt) {
+            return DBusResult{QDBusError::InternalError};
+        }
+        // postpone anything after the confirmation
+        connect(prompt, &PromptBase::completed, this, [this](bool dismissed) {
+            if (dismissed) {
+                finishPrompt(dismissed);
+            } else {
+                auto res = updateItem();
+                if (res.err()) {
+                    qWarning() << "FdoSecrets:" << res;
+                    finishPrompt(true);
+                }
+            }
+        });
+
+        auto ret = prompt->prompt(client, windowId);
         if (ret.err()) {
             return ret;
         }
-        if (locked) {
+        return {};
-            // give the user a chance to unlock the item
-            auto prompt = PromptBase::Create<UnlockPrompt>(service(), QSet<Collection*>{}, QSet<Item*>{m_item});
-            if (!prompt) {
-                return DBusResult{QDBusError::InternalError};
-            }
-            // 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;
-            }
-            return PromptResult::Pending;
-        }
-
-        // the item can be updated directly
-        return updateItem();
     }
 
     DBusResult CreateItemPrompt::updateItem()
@@ -493,6 +522,9 @@ namespace FdoSecrets
         if (ret.err()) {
             return ret;
         }
+
+        // finally can finish the prompt without dismissing it
+        finishPrompt(false);
         return {};
     }
 } // namespace FdoSecrets

src/fdosecrets/objects/Prompt.h

@@ -211,6 +211,7 @@ namespace FdoSecrets
         PromptResult promptSync(const DBusClientPtr& client, const QString& windowId) override;
         QVariant currentResult() const override;
 
+        DBusResult createItem(const QString& windowId);
         DBusResult updateItem();
 
         QPointer<Collection> m_coll;

src/fdosecrets/objects/Service.cpp

@@ -523,18 +523,37 @@ namespace FdoSecrets
             return;
         }
 
-        // mark the db as being unlocked to prevent multiple dialogs for the same db
+        // check if the db is already being unlocked to prevent multiple dialogs for the same db
         if (m_unlockingDb.contains(dbWidget)) {
             return;
         }
-        m_unlockingDb.insert(dbWidget);
 
+        // insert a dummy one here, just to prevent multiple dialogs
+        // the real one will be inserted in onDatabaseUnlockDialogFinished
+        m_unlockingDb[dbWidget] = {};
+
+        // actually show the dialog
         m_databases->unlockDatabaseInDialog(dbWidget, DatabaseOpenDialog::Intent::None);
     }
 
     void Service::onDatabaseUnlockDialogFinished(bool accepted, DatabaseWidget* dbWidget)
     {
-        m_unlockingDb.remove(dbWidget);
+        if (!m_unlockingDb.contains(dbWidget)) {
-        emit doneUnlockDatabaseInDialog(accepted, dbWidget);
+            // not our concern
+            return;
+        }
+
+        if (!accepted) {
+            emit doneUnlockDatabaseInDialog(false, dbWidget);
+            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);
+                disconnect(m_unlockingDb.take(dbWidget));
+            });
+            m_unlockingDb[dbWidget] = conn;
+        }
     }
 } // namespace FdoSecrets

src/fdosecrets/objects/Service.h

@@ -128,6 +128,7 @@ namespace FdoSecrets
     private slots:
         void ensureDefaultAlias();
 
+        void onDatabaseUnlockDialogFinished(bool accepted, DatabaseWidget* dbWidget);
         void onDatabaseTabOpened(DatabaseWidget* dbWidget, bool emitSignal);
         void monitorDatabaseExposedGroup(DatabaseWidget* dbWidget);
 
@@ -136,8 +137,6 @@ namespace FdoSecrets
 
         void onCollectionAliasRemoved(const QString& alias);
 
-        void onDatabaseUnlockDialogFinished(bool accepted, DatabaseWidget* dbWidget);
-
     private:
         bool initialize();
 
@@ -166,7 +165,8 @@ namespace FdoSecrets
         QList<Session*> m_sessions{};
 
         bool m_insideEnsureDefaultAlias{false};
-        QSet<const DatabaseWidget*> m_unlockingDb{}; // list of db being unlocking
+        // list of db currently has unlock dialog shown
+        QHash<const DatabaseWidget*, QMetaObject::Connection> m_unlockingDb{};
         QSet<const DatabaseWidget*> m_lockingDb{}; // list of db being locking
     };
 

tests/gui/TestGuiFdoSecrets.cpp

@@ -1094,6 +1094,31 @@ void TestGuiFdoSecrets::testItemCreate()
     }
 }
 
+void TestGuiFdoSecrets::testItemCreateUnlock()
+{
+    auto service = enableService();
+    VERIFY(service);
+    auto coll = getDefaultCollection(service);
+    VERIFY(coll);
+    auto sess = openSession(service, DhIetf1024Sha256Aes128CbcPkcs7::Algorithm);
+    VERIFY(sess);
+
+    // NOTE: entries are no longer valid after locking
+    lockDatabaseInBackend();
+
+    QSignalSpy spyItemCreated(coll.data(), SIGNAL(ItemCreated(QDBusObjectPath)));
+    VERIFY(spyItemCreated.isValid());
+
+    // create item
+    StringStringMap attributes{
+        {"application", "fdosecrets-test"},
+        {"attr-i[bute]", "![some] -value*"},
+    };
+
+    auto item = createItem(sess, coll, "abc", "Password", attributes, false, false, true);
+    VERIFY(item);
+}
+
 void TestGuiFdoSecrets::testItemChange()
 {
     auto service = enableService();
@@ -1678,7 +1703,8 @@ QSharedPointer<ItemProxy> TestGuiFdoSecrets::createItem(const QSharedPointer<Ses
                                                         const QString& pass,
                                                         const StringStringMap& attr,
                                                         bool replace,
-                                                        bool expectPrompt)
+                                                        bool expectPrompt,
+                                                        bool expectUnlockPrompt)
 {
     VERIFY(sess);
     VERIFY(coll);
@@ -1703,6 +1729,10 @@ QSharedPointer<ItemProxy> TestGuiFdoSecrets::createItem(const QSharedPointer<Ses
 
     // drive the prompt
     DBUS_VERIFY(prompt->Prompt(""));
+
+    bool unlockFound = driveUnlockDialog();
+    COMPARE(unlockFound, expectUnlockPrompt);
+
     bool found = driveAccessControlDialog();
     COMPARE(found, expectPrompt);
 
@@ -1800,6 +1830,9 @@ bool TestGuiFdoSecrets::driveUnlockDialog()
     processEvents();
     auto dbOpenDlg = m_tabWidget->findChild<DatabaseOpenDialog*>();
     VERIFY(dbOpenDlg);
+    if (!dbOpenDlg->isVisible()) {
+        return false;
+    }
     auto editPassword = dbOpenDlg->findChild<PasswordWidget*>("editPassword")->findChild<QLineEdit*>("passwordEdit");
     VERIFY(editPassword);
     editPassword->setFocus();

tests/gui/TestGuiFdoSecrets.h

@@ -84,6 +84,7 @@ private slots:
     void testCollectionChange();
 
     void testItemCreate();
+    void testItemCreateUnlock();
     void testItemChange();
     void testItemReplace();
     void testItemReplaceExistingLocked();
@@ -122,7 +123,8 @@ private:
                                          const QString& pass,
                                          const FdoSecrets::wire::StringStringMap& attr,
                                          bool replace,
-                                         bool expectPrompt = false);
+                                         bool expectPrompt = false,
+                                         bool expectUnlockPrompt = false);
     FdoSecrets::wire::Secret
     encryptPassword(QByteArray value, QString contentType, const QSharedPointer<SessionProxy>& sess);
     template <typename Proxy> QSharedPointer<Proxy> getProxy(const QDBusObjectPath& path) const